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 more eval functions to c-api by bschoenmaeckers · Pull Request #8244 · RustPython/RustPython 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 (1)
📝 WalkthroughWalkthroughThis PR extends the C-ABI eval and frame surface with frame execution, current-frame accessors, callable descriptor lookup, typed frame/code handles, and new FFI result conversions. ChangesC-API eval/frame extension
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
00c0b14 to
51bc472
Compare
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/ceval.rs`:
- Around line 118-124: `PyEval_GetLocals` is returning the frame’s locals
mapping without first synchronizing fast locals, so update the
`PyEval_GetLocals` implementation in `ceval.rs` to use the same synchronization
path as `frame.locals(vm)` before converting to a raw `PyObject`. Keep the
existing `with_vm`/`current_frame` flow, but fetch the synced locals mapping
from the frame rather than calling `frame.locals.as_object(vm)` directly so the
behavior matches CPython.
- Around line 127-150: PyEval_GetFuncName is returning borrowed string pointers
that may not be stable or NUL-terminated, especially for __name__ and cls.name()
paths. Update PyEval_GetFuncName to construct and return a stable C string
(matching the approach used by PyEval_GetFuncDesc) by converting the chosen name
into a CString or equivalent CStr-backed value before returning, and keep the
recursive bound_method_type path consistent with that same ownership model.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0e718ea3-ba8f-4190-9854-61beccd59be6
crates/capi/src/ceval.rscrates/capi/src/util.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.
51bc472 to
0dfff5f
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/pyframe.rs`:
- Around line 8-16: `PyFrame_GetLineNumber` currently converts the `f_lineno()`
result from `usize` to `c_int` with a blind cast, which can truncate large
values. Update the `PyFrame_GetLineNumber` implementation in `pyframe.rs` to use
a checked conversion or explicit bounds handling before returning
`core::ffi::c_int`, and keep `PyFrame_GetCode` unchanged since its ownership is
already correct.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6ac3a184-adae-42c0-8fb0-bc3796bc0001
crates/capi/src/ceval.rscrates/capi/src/pyframe.rscrates/capi/src/util.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.
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.