Add mapping c-api functions - #8003
Conversation
|
No actionable comments were generated in the recent review. π βΉοΈ Recent review infoβοΈ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: π Files selected for processing (2)
π§ Files skipped from review as they are similar to previous changes (2)
π WalkthroughWalkthroughAdds a new ChangesMapping Protocol FFI Surface
Estimated code review effortπ― 3 (Moderate) | β±οΈ ~25 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
π§Ή Nitpick comments (1)
crates/capi/src/abstract_/mapping.rs (1)
10-38: π€ Low valueConsider extracting shared logic into a helper to reduce duplication.
All three functions follow the same pattern: get mapping, call method, iterate to list. Per coding guidelines, when branches differ only in a method call but share common logic, extracting the common parts improves maintainability.
β»οΈ Possible helper extraction
+fn mapping_to_list( + obj: *mut PyObject, + get_items: impl FnOnce(&rustpython_vm::protocol::PyMapping, &rustpython_vm::VirtualMachine) -> rustpython_vm::PyResult<rustpython_vm::PyObjectRef>, +) -> *mut PyObject { + with_vm(|vm| { + let obj = unsafe { &*obj }; + let items = get_items(&obj.try_mapping(vm)?, vm)?; + let iter = items.get_iter(vm)?; + Ok(vm.ctx.new_list(iter.try_to_value(vm)?)) + }) +} + #[unsafe(no_mangle)] pub unsafe extern "C" fn PyMapping_Keys(obj: *mut PyObject) -> *mut PyObject { - with_vm(|vm| { - let obj = unsafe { &*obj }; - let keys = obj.try_mapping(vm)?.keys(vm)?; - let iter = keys.get_iter(vm)?; - Ok(vm.ctx.new_list(iter.try_to_value(vm)?)) - }) + mapping_to_list(obj, |m, vm| m.keys(vm)) }Apply similarly to
PyMapping_ValuesandPyMapping_Items.π€ Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/capi/src/abstract_/mapping.rs` around lines 10 - 38, Extract the repeated logic in PyMapping_Keys, PyMapping_Values, and PyMapping_Items into a small helper (e.g., fn map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn Mapping, &VirtualMachine) -> PyResult<PyObjectRef>) -> *mut PyObject or a helper taking an enum/closure) that: calls with_vm, dereferences obj, calls obj.try_mapping(vm)?, invokes the specific mapping method (keys/values/items) via the passed closure or selector, gets the iterator via get_iter(vm)?, converts it with try_to_value(vm) and returns vm.ctx.new_list(...); then replace the bodies of PyMapping_Keys, PyMapping_Values and PyMapping_Items to call this helper with the appropriate mapping method selector so the only differing part is the method passed in.
π€ Prompt for all review comments with AI agents
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_/mapping.rs`:
- Around line 40-66: Change the test module attribute from the disabled
#[cfg(false)] to #[cfg(test)] (or #[cfg(test)] plus #[ignore] if you want it
compiled but skipped), and update the test function size_keys_values_items to
call the RustPython C-API wrappers instead of only exercising PyO3: keep
Python::attach and creation of the PyDict/PyMapping, then invoke the C-API
functions (e.g., PyMapping_Size, PyMapping_Keys, PyMapping_Values,
PyMapping_Items or their crate wrapper functions) against the mapping and assert
their return values/lengths and that items are tuples, so the test actually
validates the C-API implementation rather than only PyO3 helpers.
---
Nitpick comments:
In `@crates/capi/src/abstract_/mapping.rs`:
- Around line 10-38: Extract the repeated logic in PyMapping_Keys,
PyMapping_Values, and PyMapping_Items into a small helper (e.g., fn
map_to_list(obj: *mut PyObject, f: impl FnOnce(&dyn Mapping, &VirtualMachine) ->
PyResult<PyObjectRef>) -> *mut PyObject or a helper taking an enum/closure)
that: calls with_vm, dereferences obj, calls obj.try_mapping(vm)?, invokes the
specific mapping method (keys/values/items) via the passed closure or selector,
gets the iterator via get_iter(vm)?, converts it with try_to_value(vm) and
returns vm.ctx.new_list(...); then replace the bodies of PyMapping_Keys,
PyMapping_Values and PyMapping_Items to call this helper with the appropriate
mapping method selector so the only differing part is the method passed in.
πͺ Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
βΉοΈ Review info
βοΈ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 1d45ea1b-0816-4cf2-9aa0-8956af0e0836
π Files selected for processing (2)
crates/capi/src/abstract_.rscrates/capi/src/abstract_/mapping.rs
5cb4197 to
5bc9850
Compare
Head branch was pushed to by a user without write access
5bc9850 to
a7c74a9
Compare
Used AI to implement the mapping protocol c-api.
Summary by CodeRabbit