GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Add number protocol functions to c-api by bschoenmaeckers · Pull Request #8005 · RustPython/RustPython

Conversation

@bschoenmaeckers

@bschoenmaeckers bschoenmaeckers commented Jun 1, 2026 •
edited by coderabbitai Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • New Features

    • Numeric operations are exposed in the C API: addition, subtraction, left/right shifts, bitwise OR, and indexing; the numeric module is publicly available.
  • Tests

    • Unit tests for these numeric behaviors remain in the codebase but are disabled and not executed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@coderabbitai

coderabbitai Bot commented Jun 1, 2026 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a number C-API module and re-export; implements six PyNumber_* extern "C" FFI functions that call into the VM for numeric/bitwise operations. A test module with integer-operation checks exists but is disabled via #[cfg(false)].

Changes

PyNumber FFI Bindings

Layer / File(s) Summary
FFI implementations and module wiring
crates/capi/src/abstract_.rs, crates/capi/src/abstract_/number.rs (lines 1–31)
Declares the number module and pub use number::*; adds #[no_mangle] pub unsafe extern "C" functions PyNumber_Add, PyNumber_Index, PyNumber_Lshift, PyNumber_Or, PyNumber_Rshift, and PyNumber_Subtract that call into the VM via with_vm and perform unsafe pointer dereference.
Number operation tests (disabled)
crates/capi/src/abstract_/number.rs (lines 33–86)
Test module constructs Python integers and asserts extracted i64 results for add, lshift, bitor, rshift, sub, but the module is guarded with #[cfg(false)] so it is not compiled or executed.

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
Loading

Estimated Code Review Effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • RustPython/RustPython#7882: Adds C-API extern "C" exports under abstract_.rs, related to adding PyNumber_* entrypoints in this PR.

Suggested Reviewers

  • youknowone

Poem

🐰 I nibble at bindings, bits, and sum,
I bridge raw pointers so calls may run.
Add, shift, and or now routed through,
Silent tests wait, not yet in view. 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: adding number protocol functions to the C API.
Docstring Coverage ✅ Passed Docstring coverage is 91.67% which is sufficient. The required threshold is 80.00%.
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
  • X
  • Mastodon
  • Reddit
  • LinkedIn

Comment @coderabbitai help to get the list of available commands and usage tips.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bschoenmaeckers bschoenmaeckers changed the title Add number protocol functions Add number protocol functions to c-api Jun 1, 2026
ShaharNaveh
ShaharNaveh approved these changes Jun 1, 2026
View reviewed changes

@ShaharNaveh ShaharNaveh left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
coderabbitai[bot]
coderabbitai Bot reviewed Jun 1, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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_/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).
🪄 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: f91a989d-6d34-44e5-8c47-8561f475250d

📥 Commits

Reviewing files that changed from the base of the PR and between 1ce37bf and d5643f3.

📒 Files selected for processing (2)
  • crates/capi/src/abstract_.rs
  • crates/capi/src/abstract_/number.rs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/capi/src/abstract_/number.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread crates/capi/src/abstract_/number.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

@bschoenmaeckers

bschoenmaeckers commented Jun 1, 2026

Copy link
Copy Markdown
Contributor Author

@coderabbitai autofix

coderabbitai[bot] reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@coderabbitai

coderabbitai Bot commented Jun 1, 2026

Copy link
Copy Markdown
Contributor

This command requires write access to the repository. Only users with write or admin permissions can trigger CodeRabbit to commit or create pull requests.

bschoenmaeckers reacted with confused emoji
All reactions
  • 😕 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bschoenmaeckers
bschoenmaeckers marked this pull request as draft June 1, 2026 15:31
@bschoenmaeckers
bschoenmaeckers force-pushed the c-api-number branch from d5643f3 to 4f64690 Compare June 1, 2026 16:51
@bschoenmaeckers
bschoenmaeckers marked this pull request as ready for review June 1, 2026 16:51
@bschoenmaeckers
bschoenmaeckers force-pushed the c-api-number branch from 4f64690 to 82c8f92 Compare June 1, 2026 17:25
coderabbitai[bot]
coderabbitai Bot reviewed Jun 1, 2026
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 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_/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.
🪄 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: a841889b-bf28-4038-bf28-ea5973f3bbcd

📥 Commits

Reviewing files that changed from the base of the PR and between 4f64690 and 82c8f92.

📒 Files selected for processing (2)
  • crates/capi/src/abstract_.rs
  • crates/capi/src/abstract_/number.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/capi/src/abstract_.rs

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread crates/capi/src/abstract_/number.rs Resolved

Uh oh!

There was an error while loading. Please reload this page.

youknowone
youknowone approved these changes Jun 2, 2026
View reviewed changes

@youknowone youknowone left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 please rebase this

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bschoenmaeckers
bschoenmaeckers force-pushed the c-api-number branch from 82c8f92 to 6f43fe6 Compare June 2, 2026 07:25
@bschoenmaeckers

bschoenmaeckers commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

👍 please rebase this

Done

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bschoenmaeckers
bschoenmaeckers force-pushed the c-api-number branch from 6f43fe6 to 9cb27ef Compare June 2, 2026 08:31
@youknowone
youknowone enabled auto-merge (squash) June 2, 2026 09:08
@bschoenmaeckers
Add number protocol functions
d9c591b
auto-merge was automatically disabled June 2, 2026 09:35

Head branch was pushed to by a user without write access

@bschoenmaeckers
bschoenmaeckers force-pushed the c-api-number branch from 9cb27ef to d9c591b Compare June 2, 2026 09:35
@bschoenmaeckers

bschoenmaeckers commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author

Rebased, please try to merge again 🙏

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@youknowone
youknowone merged commit 60b1346 into RustPython:main Jun 3, 2026
26 checks passed

Uh oh!

There was an error while loading. Please reload this page.

@bschoenmaeckers
bschoenmaeckers deleted the c-api-number branch June 3, 2026 09:38
@coderabbitai coderabbitai Bot mentioned this pull request Jun 24, 2026
Add more number functions to c-api #8158
Merged
youknowone pushed a commit that referenced this pull request Sep 16, 2026
@bschoenmaeckers
Add number protocol functions (#8005)
7f97f9b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@youknowone youknowone youknowone approved these changes
@ShaharNaveh ShaharNaveh ShaharNaveh approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@bschoenmaeckers @youknowone @ShaharNaveh
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.