GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||
GeezerVison is showing github.com — Add number protocol functions to c-api by bschoenmaeckers · Pull Request #8005 · RustPython/RustPython Conversation
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a ChangesPyNumber FFI Bindings
Sequence Diagram(s)sequenceDiagram
participant C as C caller
participant FFI as PyNumber_* (FFI)
participant VM as RustPython VM
C->>FFI: PyNumber_Add(PyObject*, PyObject*)
FFI->>VM: with_vm -> vm._add(&*o1, &*o2)
VM-->>FFI: *mut PyObject
FFI-->>C: *mut PyObject
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
🚀
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/abstract_/number.rs`:
- Around line 33-86: The tests currently exercise Python-level methods but not
the exported C-ABI functions, so add unit tests that call the no_mangle extern
"C" symbols directly (PyNumber_Add, PyNumber_Lshift, PyNumber_Or,
PyNumber_Rshift, PyNumber_Subtract and PyNumber_Index) via the pyo3 ffi
pointers; inside Python::with_gil (or Python::attach) obtain raw PyObject
pointers for lhs/rhs, call the corresponding pyo3::ffi::PyNumber_* function in
an unsafe block, check the returned *mut pyo3::ffi::PyObject is non-null, wrap
or convert it back to a PyObject and extract the i64/expected value, and DECREF
returned objects as appropriate to avoid leaks so these tests truly validate the
exported C-ABI entrypoints.
- Around line 3-31: The FFI entry points PyNumber_Add, PyNumber_Index,
PyNumber_Lshift, PyNumber_Or, PyNumber_Rshift, and PyNumber_Subtract perform raw
pointer derefs and must be declared unsafe; update each function signature from
the current extern "C" form to be pub unsafe extern "C" fn while preserving the
no_mangle attribute (or replacing #[no_mangle] if present) so the functions are
exported with the correct caller safety contract (adjust signatures for
PyNumber_Add, PyNumber_Index, PyNumber_Lshift, PyNumber_Or, PyNumber_Rshift,
PyNumber_Subtract).
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f91a989d-6d34-44e5-8c47-8561f475250d
crates/capi/src/abstract_.rscrates/capi/src/abstract_/number.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
@coderabbitai autofix |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
d5643f3 to
4f64690
Compare
4f64690 to
82c8f92
Compare
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/abstract_/number.rs`:
- Around line 33-34: The test module is permanently disabled by #[cfg(false]);
change it to #[cfg(test)] so tests are compiled and run, then update the tests
in mod tests to exercise the C ABI entrypoints for the new PyNumber_* functions
(call the exported FFI wrappers rather than internal helpers) and adjust any
signatures/unsafe blocks to match the C-ABI bindings (e.g., use the public
extern "C" functions named PyNumber_*) so the tests actually invoke the FFI
layer; alternatively, if you intend to defer tests, remove the dead mod tests
block entirely.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: a841889b-bf28-4038-bf28-ea5973f3bbcd
crates/capi/src/abstract_.rscrates/capi/src/abstract_/number.rs
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
👍 please rebase this
Sorry, something went wrong.
There was an error while loading. Please reload this page.
82c8f92 to
6f43fe6
Compare
Done |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
6f43fe6 to
9cb27ef
Compare
Head branch was pushed to by a user without write access
9cb27ef to
d9c591b
Compare
|
Rebased, please try to merge again 🙏 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.