os.waitstatus_to_exitcode for windows #6355
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRefactors Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
crates/vm/src/stdlib/os.rs (1)
1442-1450: LGTM! Windows implementation correctly extracts exit code.The
>> 8shift and u32 validation match CPython's Windows behavior. The distinct signature (u64 input, u32 output) appropriately reflects Windows semantics where exit codes are always non-negative.Nitpick: Minor inconsistency in error message capitalization—line 1439 uses "Invalid" while line 1449 uses "invalid". Consider aligning for consistency.
- .map_err(|_| vm.new_value_error(format!("invalid exit code: {exitcode}"))) + .map_err(|_| vm.new_value_error(format!("Invalid exit code: {exitcode}")))
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Lib/test/test_os.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/stdlib/os.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/os.rs
🧠 Learnings (2)
📓 Common learnings
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration rather than providing fallback values for other platforms.
📚 Learning: 2025-06-27T14:47:28.810Z
Learnt from: moreal
Repo: RustPython/RustPython PR: 5847
File: vm/src/stdlib/stat.rs:547-567
Timestamp: 2025-06-27T14:47:28.810Z
Learning: In RustPython's stat module implementation, platform-specific constants like SF_SUPPORTED and SF_SYNTHETIC should be conditionally declared only for the platforms where they're available (e.g., macOS), following CPython's approach of optional declaration using #ifdef checks rather than providing fallback values for other platforms.
Applied to files:
crates/vm/src/stdlib/os.rs
🔇 Additional comments (1)
crates/vm/src/stdlib/os.rs (1)
1424-1440: LGTM! Unix implementation correctly handles wait status.The implementation properly uses libc macros (WIFEXITED/WEXITSTATUS/WIFSIGNALED/WTERMSIG) and correctly returns negative values for signal-terminated processes. The validation flow (i32 → u32 → c_int) ensures non-negative input while maintaining compatibility with libc macro expectations.
Based on learnings, this follows the recommended pattern of platform-specific conditional compilation.
youknowone
changed the title
1 participant
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.