Newtype SignalHandlers by ShaharNaveh · Pull Request #8089 · RustPython/RustPython

ShaharNaveh

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 32b83d06-3524-45ed-89fc-258b5ac0515a

📥 Commits

Reviewing files that changed from the base of the PR and between d18d35f and 9198c26.

📒 Files selected for processing (4)

crates/vm/src/signal.rs
crates/vm/src/stdlib/_signal.rs
crates/vm/src/stdlib/posix.rs
crates/vm/src/vm/mod.rs

📝 Walkthrough

Walkthrough

The PR refactors signal handler management in RustPython's VM by introducing a SignalHandlers wrapper struct to encapsulate boxed refcell signal storage, replacing the old new_signal_handlers() helper function. The VM's signal_handlers field type changes to OnceCell<SignalHandlers>, and all call sites—including initialization, signal stdlib operations, and post-fork handling—are updated to use the new wrapper pattern.

Changes

Signal Handlers Wrapper and VM Integration

Layer / File(s)	Summary
SignalHandlers wrapper type definition `crates/vm/src/signal.rs`	New `SignalHandlers` struct wraps `Box<RefCell<[Option<PyObjectRef>; NSIG]>>` with `Default`, `Deref`, and `DerefMut` trait implementations. Imports use `core::fmt` instead of `alloc::fmt`. Removes the old `new_signal_handlers()` helper function.
VirtualMachine signal_handlers field and initialization `crates/vm/src/vm/mod.rs`	Imports `SignalHandlers` from signal module. Changes `signal_handlers` field type from raw boxed refcell array to `OnceCell<SignalHandlers>`. Initializes the field with `SignalHandlers::default()` wrapped in `OnceCell::from()`.
Signal stdlib module initialization and accessors `crates/vm/src/stdlib/_signal.rs`	Imports `SignalHandlers` in addition to signal module. Replaces `signal::new_signal_handlers()` calls with `SignalHandlers::default` in `init_signal_handlers()`, `signal()`, and `getsignal()` functions.
Post-fork signal handler reinitialization `crates/vm/src/stdlib/posix.rs`	Updates `py_os_after_fork_child` to use `SignalHandlers::default` for lazy initialization in forked child processes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Suggested reviewers

youknowone

Poem

🐰 Signals now wrapped with care and grace,
A SignalHandlers in its rightful place,
From helper gone to struct so neat,
The refactor's dance is now complete! 🎯

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 62.50% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'Newtype SignalHandlers' directly and accurately describes the main change: introducing a new `SignalHandlers` newtype wrapper struct around signal-handler storage, which is the primary refactoring across all modified files.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests

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.}