os.waitstatus_to_exitcode for windows by youknowone · Pull Request #6355 · RustPython/RustPython

youknowone

Caution

Review failed

The pull request is closed.

Walkthrough

Refactors waitstatus_to_exitcode into platform-specific implementations: a Unix variant under #[cfg(unix)] using i32 and WIFEXITED/WEXITSTATUS/WIFSIGNALED, and a new Windows variant accepting u64 that extracts the exit code via bit-shifting and validates it as u32. (47 words)

Changes

Cohort / File(s)	Change Summary
Platform-specific waitstatus_to_exitcode refactor `crates/vm/src/stdlib/os.rs`	Split the previous cross-platform function into two platform-guarded implementations: `#[cfg(unix)]` function uses `status: i32` with WIFEXITED/WIFEXITSTATUS/WIFSIGNALED logic returning `PyResult<i32>`; a Windows-only function uses `status: u64`, extracts the exit code via `status >> 8`, validates it fits in `u32`, and returns `PyResult<u32>`. Removed prior combined cfg_if Windows handling from the Unix-guarded path.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Single-file, focused platform split with small type/signature changes.
Pay attention to:
- Call sites expecting the old signature/type.
- Correct cfg attributes and exported visibility.
- Consistent error handling and return types between platforms.

Possibly related PRs

os.waitstatus_to_exitcode for windows #6355: Modifies the same waitstatus_to_exitcode function and splits Unix/Windows implementations — directly related.
Windows execv, spawnv, wait #6350: Changes wait/waitpid behavior to return u64, aligning with the new Windows signature that accepts u64.

Poem

🐰
Hops through code on nimble feet,
Unix paths and Windows meet,
Bits shift, macros softly sing,
Each platform gets its proper wing,
A carrot cheer for builds complete 🥕

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check	✅ Passed	The title 'os.waitstatus_to_exitcode for windows' accurately describes the main change: adding Windows support to the os.waitstatus_to_exitcode function by implementing a Windows-specific variant.

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c5b2b41 and f22bac7.

📒 Files selected for processing (1)

crates/vm/src/stdlib/os.rs (1 hunks)

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}