feat(stable-api): add integer conversion methods #676
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ianks
force-pushed
the
feat/stable-api-integers
branch
4 times, most recently
from
91d0986 to
b53819b
Compare
This commit adds comprehensive integer and symbol conversion support to the stable API, enabling safe Ruby <-> C type conversions across all Ruby versions. Integer Conversions: - FIX2LONG/FIX2ULONG - Extract values from Fixnum - LONG2FIX - Create Fixnum from long (with range check via FIXABLE) - NUM2LONG/NUM2ULONG - Convert any Integer (Fixnum or Bignum) to C types - LONG2NUM/ULONG2NUM - Create Integer from C types (Fixnum or Bignum) - FIXABLE/POSFIXABLE - Range checking for safe Fixnum creation Symbol Conversions: - ID2SYM/RB_ID2SYM - Convert ID to Symbol - SYM2ID/RB_SYM2ID - Convert Symbol to ID Implementation: - Added trait methods to StableApiDefinition - Rust implementations for all Ruby versions (2.7, 3.0, 3.1, 3.2, 3.3, 3.4, 4.0) - C fallback implementations via compiled.c - Public macros in macros.rs mirroring Ruby C API - Comprehensive test coverage (128 tests) - Performance analysis support via enhanced show-asm script All implementations maintain parity between Rust and compiled C versions, verified through extensive parity testing.
ianks
force-pushed
the
feat/stable-api-integers
branch
from
b53819b to
d7fe290
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds stable API integer conversion functions to enable efficient conversion between Ruby Fixnum/Integer values and C integer types. The implementation provides 9 conversion and validation methods implemented using bit manipulation for fixnum operations with C fallback implementations for reference testing.
Key changes include:
- Core conversion functions (
fix2long,fix2ulong,long2fix,long2num,ulong2num) - Validation functions (
fixable,posfixable) - Numeric conversion with Bignum support (
num2long,num2ulong) - Platform-specific test handling for Windows (32-bit c_long) vs Unix (64-bit c_long)
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
crates/rb-sys/src/stable_api.rs |
Adds 9 new trait method signatures for integer conversion with safety documentation |
crates/rb-sys/src/stable_api/ruby_2_7.rs |
Implements integer conversion methods for Ruby 2.7 using bit manipulation |
crates/rb-sys/src/stable_api/ruby_3_1.rs |
Implements integer conversion methods for Ruby 3.1 using bit manipulation |
crates/rb-sys/src/stable_api/ruby_3_2.rs |
Implements integer conversion methods for Ruby 3.2 using bit manipulation |
crates/rb-sys/src/stable_api/ruby_3_3.rs |
Implements integer conversion methods for Ruby 3.3 using bit manipulation |
crates/rb-sys/src/stable_api/ruby_3_4.rs |
Implements integer conversion methods for Ruby 3.4 using bit manipulation |
crates/rb-sys/src/stable_api/ruby_4_0.rs |
Implements integer conversion methods for Ruby 4.0 plus unrelated type_p refactoring |
crates/rb-sys/src/stable_api/compiled.c |
Adds C reference implementations using Ruby's standard macros |
crates/rb-sys/src/stable_api/compiled.rs |
Adds FFI bindings to C implementations for parity testing |
crates/rb-sys/src/macros.rs |
Exposes public macro wrappers with #[inline(always)] for all conversion functions |
crates/rb-sys-tests/src/stable_api_test.rs |
Adds comprehensive parity tests with platform-specific boundary testing |
script/show-asm |
Adds assembly inspection configurations for new conversion operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+350
to
+414
| fn fix2long(&self, obj: VALUE) -> std::os::raw::c_long { | ||
| // Extract the integer value by performing an arithmetic right shift by 1 | ||
| (obj as std::os::raw::c_long) >> 1 | ||
| } | ||
|
|
||
| #[inline] | ||
| fn fix2ulong(&self, obj: VALUE) -> std::os::raw::c_ulong { | ||
| // For positive fixnums, cast to c_long then to c_ulong | ||
| ((obj as std::os::raw::c_long) >> 1) as std::os::raw::c_ulong | ||
| } | ||
|
|
||
| #[inline] | ||
| fn long2fix(&self, val: std::os::raw::c_long) -> VALUE { | ||
| // Left shift by 1 and OR with FIXNUM_FLAG | ||
| (((val as VALUE) << 1) | crate::FIXNUM_FLAG as VALUE) as VALUE | ||
| } | ||
|
|
||
| #[inline] | ||
| fn fixable(&self, val: std::os::raw::c_long) -> bool { | ||
| // Check if value is within Fixnum range | ||
| val >= crate::special_consts::FIXNUM_MIN && val <= crate::special_consts::FIXNUM_MAX | ||
| } | ||
|
|
||
| #[inline] | ||
| fn posfixable(&self, val: std::os::raw::c_ulong) -> bool { | ||
| // Check if unsigned value fits in positive fixnum | ||
| val <= crate::special_consts::FIXNUM_MAX as std::os::raw::c_ulong | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn num2long(&self, obj: VALUE) -> std::os::raw::c_long { | ||
| if self.fixnum_p(obj) { | ||
| self.fix2long(obj) | ||
| } else { | ||
| crate::rb_num2long(obj) | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| unsafe fn num2ulong(&self, obj: VALUE) -> std::os::raw::c_ulong { | ||
| if self.fixnum_p(obj) { | ||
| self.fix2ulong(obj) | ||
| } else { | ||
| crate::rb_num2ulong(obj) | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| fn long2num(&self, val: std::os::raw::c_long) -> VALUE { | ||
| if self.fixable(val) { | ||
| self.long2fix(val) | ||
| } else { | ||
| unsafe { crate::rb_int2big(val as isize) } | ||
| } | ||
| } | ||
|
|
||
| #[inline] | ||
| fn ulong2num(&self, val: std::os::raw::c_ulong) -> VALUE { | ||
| if self.posfixable(val) { | ||
| self.long2fix(val as std::os::raw::c_long) | ||
| } else { | ||
| unsafe { crate::rb_uint2big(val as usize) } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Inconsistent inline annotation: The PR description states "Replace #[inline] with #[inline(always)]" for numeric conversion helpers, but the actual implementation uses #[inline] instead of #[inline(always)]. While #[inline] may be sufficient and more appropriate (allowing the compiler to decide), this is inconsistent with the stated design decision in the PR description.
Comment on lines
173
to
198
| use crate::ruby_special_consts::*; | ||
| use crate::ruby_value_type::*; | ||
|
|
||
| if t == RUBY_T_TRUE { | ||
| obj == RUBY_Qtrue as _ | ||
| } else if t == RUBY_T_FALSE { | ||
| obj == RUBY_Qfalse as _ | ||
| } else if t == RUBY_T_NIL { | ||
| obj == RUBY_Qnil as _ | ||
| } else if t == RUBY_T_UNDEF { | ||
| obj == RUBY_Qundef as _ | ||
| } else if t == RUBY_T_FIXNUM { | ||
| self.fixnum_p(obj) | ||
| } else if t == RUBY_T_SYMBOL { | ||
| self.symbol_p(obj) | ||
| } else if t == RUBY_T_FLOAT { | ||
| self.float_type_p(obj) | ||
| } else if self.special_const_p(obj) { | ||
| false | ||
| if !self.special_const_p(obj) { | ||
| self.builtin_type(obj) == ty | ||
| } else if obj == RUBY_Qfalse as _ { | ||
| ty == RUBY_T_FALSE | ||
| } else if obj == RUBY_Qnil as _ { | ||
| ty == RUBY_T_NIL | ||
| } else if obj == RUBY_Qtrue as _ { | ||
| ty == RUBY_T_TRUE | ||
| } else if obj == RUBY_Qundef as _ { | ||
| ty == RUBY_T_UNDEF | ||
| } else if self.fixnum_p(obj) { | ||
| ty == RUBY_T_FIXNUM | ||
| } else if self.static_sym_p(obj) { | ||
| ty == RUBY_T_SYMBOL | ||
| } else if self.flonum_p(obj) { | ||
| ty == RUBY_T_FLOAT | ||
| } else { | ||
| // Optimized: For heap objects, directly compare builtin_type | ||
| // This eliminates the extra check in the original: else if t == self.builtin_type(obj) | ||
| t == self.builtin_type(obj) | ||
| // This eliminates the extra check in the original: else if ty == self.builtin_type(obj) | ||
| ty == self.builtin_type(obj) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Unrelated refactoring: This PR includes a refactoring of the type_p method (renaming parameter from t to ty and restructuring the logic) which is not mentioned in the PR description or related to adding integer conversion methods. This change should either be mentioned in the PR description or moved to a separate PR for clarity.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Adds stable API integer conversion functions with comprehensive testing and Windows platform compatibility fixes. This enables efficient conversion between Ruby Fixnum/Integer values and C integer types without relying on Ruby's internal macros.
Changes
Core Features
Integer Conversion Methods: Add 9 methods to
StableApiDefinitiontrait:fix2long/fix2ulong- Convert Fixnum VALUE to C long/ulonglong2fix- Convert C long to Fixnum VALUEfixable/posfixable- Check if value fits in Fixnum rangenum2long/num2ulong- Convert any Ruby numeric to C long/ulonglong2num/ulong2num- Convert C long/ulong to Ruby IntegerVersion Support: Implement for all Ruby versions (2.7, 3.0-3.4, 4.0)
Implementation Variants:
compiled.cfor reference and parity testingFIX2LONG,NUM2LONG, etc.) inmacros.rsTesting
Windows Platform Compatibility
#[cfg(not(windows))]gates for 64-bit specific boundary tests#[cfg(windows)]variants with appropriate 32-bit valuesPerformance Optimizations
#[inline]with#[inline(always)]for numeric conversion helpersImplementation Details
FIXNUM_FLAGto create fixnum tagrb_num2long()for BignumLONG2FIXif value isFIXABLE, else callsrb_int2big()Files Changed
crates/rb-sys/src/macros.rs- Public macro wrapperscrates/rb-sys/src/stable_api.rs- Trait definitionscrates/rb-sys/src/stable_api/compiled.c- C reference implementationscrates/rb-sys/src/stable_api/compiled.rs- Compiled C bindingscrates/rb-sys/src/stable_api/ruby_*.rs- Version-specific implementations (2.7, 3.0-3.4, 4.0)crates/rb-sys-tests/src/stable_api_test.rs- Comprehensive test suitescript/show-asm- Assembly inspection script enhancementsCloses