feat(fromv8/tov8): add more number types & support bigint #1249
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (4)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request expands numeric ToV8/FromV8 support to include u64, i64, usize, isize, and f64, and adds a public Sequence Diagram(s)sequenceDiagram
participant Rust
participant BigInt Conversion
participant V8 Runtime
Note over Rust,V8 Runtime: BigInt -> V8 (ToV8)
Rust->>BigInt Conversion: BigInt { sign_bit, words }
BigInt Conversion->>V8 Runtime: v8::BigInt::new_from_words(sign_bit, words)
V8 Runtime-->>BigInt Conversion: v8::Local<v8::BigInt> or error
BigInt Conversion-->>Rust: result
Note over Rust,V8 Runtime: V8 -> BigInt (FromV8)
V8 Runtime->>BigInt Conversion: v8::Local<v8::BigInt>
BigInt Conversion->>BigInt Conversion: extract sign_bit & words
BigInt Conversion-->>Rust: BigInt { sign_bit, words }
Estimated code review effort🎯 High | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/convert.rs(1 hunks)ops/conversion/from_v8/struct.rs(3 hunks)ops/conversion/to_v8/struct.rs(3 hunks)ops/lib.rs(2 hunks)ops/webidl/dictionary.rs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
ops/conversion/to_v8/struct.rs (2)
ops/lib.rs (1)
new(69-74)core/fast_string.rs (1)
new(30-32)
ops/conversion/from_v8/struct.rs (2)
ops/lib.rs (1)
new(69-74)core/fast_string.rs (1)
new(30-32)
ops/lib.rs (1)
core/fast_string.rs (2)
v8_string(49-54)v8_string(300-318)
core/convert.rs (3)
core/runtime/ops_rust_to_v8.rs (9)
to_v8(37-40)to_v8(46-46)to_v8(77-85)to_v8(449-456)to_v8(463-470)to_v8(478-483)to_v8(575-580)to_v8(592-594)to_v8(602-611)core/runtime/ops.rs (1)
to_v8(2474-2479)serde_v8/magic/bigint.rs (1)
words(62-62)
🔇 Additional comments (6)
core/convert.rs (2)
300-302: LGTM!Expanding the numeric types to include
u64,i64,usize,isize, andf64is a sensible addition for broader interop coverage.
304-320: LGTM!The
BigIntstruct and itsToV8implementation look correct. Usingv8::BigInt::new_from_wordswith proper error handling is the right approach.ops/lib.rs (1)
63-94: LGTM!Clean refactor from
V8EternaltoV8StaticString. The static string approach is simpler than the previous lazy thread-local initialization. The naming convention__v8_static_{name}is clear and theget_keymethod correctly delegates tov8_string.ops/conversion/from_v8/struct.rs (1)
5-5: LGTM!The transition from
V8EternaltoV8StaticStringis applied consistently. The field handling and initialization paths are correctly updated.Also applies to: 160-160, 216-216
ops/conversion/to_v8/struct.rs (1)
3-3: LGTM!Consistent application of the
V8StaticStringrefactor, mirroring the changes infrom_v8/struct.rs.Also applies to: 107-107, 143-143
ops/webidl/dictionary.rs (1)
4-4: LGTM!The
V8StaticStringtransition is consistently applied to the WebIDL dictionary field handling, aligning with the changes across the other conversion modules.Also applies to: 167-167, 233-233
| } | ||
| }).map(Into::into) | ||
| } | ||
| #static_name.v8_string(__scope).map(Into::into) |
There was a problem hiding this comment.
Are we sure that this is the fastest way? Maybe this should be written manually to use internalized string? @nathanwhit suggests that it's the fastest way to serialize the string
There was a problem hiding this comment.
i'm not sure, maybe worth benchmarking? its worth noting that this only has to be faster than serde_v8.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/convert.rs (1)
304-307: Consider adding standard derives for consistency and usability.Other wrapper types in this file (e.g.,
Smi,Number,OptionNull) include derives likeDebug,Clone,PartialEq,Eq. Adding them here would improve ergonomics for users.+#[derive(Debug, Clone, PartialEq, Eq)] pub struct BigInt { pub sign_bit: bool, pub words: Vec<u64>, }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
core/convert.rs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
core/convert.rs (1)
serde_v8/magic/bigint.rs (1)
words(62-62)
🔇 Additional comments (3)
core/convert.rs (3)
300-302: LGTM!Expanding the numeric types to include u64, i64, usize, isize, and f64 is a useful addition. Note that for values exceeding 2^53, precision will be lost due to JavaScript's f64-based Number type—users requiring exact large integers should use
BigIntinstead.
322-337: LGTM!The
FromV8implementation correctly allocates the vector with proper length usingvec![0u64; word_count], addressing the prior review feedback about theVec::with_capacitybug.
1520-1600: Good test coverage for BigInt.Tests cover positive, negative, zero, large multi-word values (2^64), and round-trip conversion. This will effectively guard against regressions like the prior
Vec::with_capacitybug.
also removes eternals usage as discussed with @devsnek