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