-
Notifications
You must be signed in to change notification settings - Fork 1.4k
impl {raise_,str}signal #6411
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
impl {raise_,str}signal #6411
Conversation
WalkthroughThe PR extends the signal handling module with three new cross-platform utility functions (raise_signal, strsignal, valid_signals) featuring platform-specific implementations, modifies set_wakeup_fd to return i64 with Windows validation, and adds year validation for time formatting on Windows. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Code has been automatically formatted The code in this PR has been formatted using git pull origin signal |
8cab405 to
b11c53b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/stdlib/signal.rs (1)
230-311: Windows: use safei64conversion for returned previous wakeup fd to prevent overflowOn Windows,
old_fdis ausize(fromAtomicUsize.swap()), and the direct castOk(old_fd as i64)at line 304 can silently overflow or truncate if the socket value exceedsi64::MAX. Usei64::try_from()with proper error handling instead.#[cfg(windows)] { if old_fd == INVALID_WAKEUP { Ok(-1) } else { - Ok(old_fd as i64) + i64::try_from(old_fd) + .map_err(|_| vm.new_value_error("previous wakeup fd does not fit in i64")) } }
🧹 Nitpick comments (2)
crates/vm/src/stdlib/time.rs (1)
354-381: Fix platform-specific error message + avoid false positives for literal%%y
- The guard is compiled for Windows/AIX/Solaris (Line 365), but the message says “on Windows” (Line 367).
contains("%y")(Line 366) will also trip on literal%%y(should be treated as a literal%y, not a directive).- // On Windows/AIX/Solaris, %y format with year < 1900 is not supported + // On Windows/AIX/Solaris, %y with year < 1900 is not supported #[cfg(any(windows, target_os = "aix", target_os = "solaris"))] - if instant.year() < 1900 && format.as_str().contains("%y") { - let msg = "format %y requires year >= 1900 on Windows"; + if instant.year() < 1900 && has_strftime_directive(format.as_str(), 'y') { + let msg = "format %y requires year >= 1900 on this platform"; return Err(vm.new_value_error(msg.to_owned())); } + + #[cfg(any(windows, target_os = "aix", target_os = "solaris"))] + fn has_strftime_directive(fmt: &str, directive: char) -> bool { + // minimal scan to ignore escaped percent (%%) + let mut it = fmt.chars().peekable(); + while let Some(c) = it.next() { + if c != '%' { + continue; + } + match it.next() { + Some('%') | None => continue, // literal '%' + Some(d) if d == directive => return true, + Some(_) => continue, + } + } + false + }crates/vm/src/stdlib/signal.rs (1)
325-358:raise_signal: prefer errno/last_os_error-based exception on failure (and dedupe Windows valid-signal list)
libc::raise()failure currently returnsnew_os_error("raise_signal failed ...")(Line 351) but doesn’t include the platform error detail. Also, the Windows allowed-signal list is duplicated withvalid_signals.let res = unsafe { libc::raise(signalnum) }; if res != 0 { - return Err(vm.new_os_error(format!("raise_signal failed for signal {}", signalnum))); + return Err(std::io::Error::last_os_error().into_pyexception(vm)); }
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
Lib/test/test_regrtest.pyis excluded by!Lib/**Lib/test/test_signal.pyis excluded by!Lib/**Lib/test/test_strftime.pyis excluded by!Lib/**
📒 Files selected for processing (2)
crates/vm/src/stdlib/signal.rs(4 hunks)crates/vm/src/stdlib/time.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.rs: Follow the default rustfmt code style by runningcargo fmtto format Rust code
Always run clippy to lint Rust code (cargo clippy) before completing tasks and fix any warnings or lints introduced by changes
Follow Rust best practices for error handling and memory management
Use the macro system (pyclass,pymodule,pyfunction, etc.) when implementing Python functionality in Rust
Files:
crates/vm/src/stdlib/time.rscrates/vm/src/stdlib/signal.rs
🧬 Code graph analysis (1)
crates/vm/src/stdlib/signal.rs (3)
crates/common/src/crt_fd.rs (1)
try_borrow_raw(261-267)crates/common/src/fileutils.rs (4)
fstat(11-21)fstat(95-159)std(316-324)std(334-334)crates/vm/src/vm/mod.rs (1)
check_signals(778-787)
🔇 Additional comments (2)
crates/vm/src/stdlib/signal.rs (2)
395-434:valid_signals: LGTM (platform-specific set contents match intent)The platform split (Unix: 1..NSIG skipping SIGKILL/SIGSTOP; Windows: fixed subset; others: empty) is clear and matches the stated goals.
360-394: Review comment is incorrect; RustPython implementation already matches CPython behaviorCPython's
signal.strsignal()raisesValueErrorfor out-of-range/invalid signal numbers, not returnsNone. The function returnsNoneonly for valid signal numbers that lack a platform-specific description. RustPython's current implementation is correct and compatible with CPython; the suggested changes would introduce incompatibility.Likely an incorrect or invalid review comment.
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.