correct addition coercion and optimize pooled arithmetic - #61
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughBigInt arithmetic now reduces intermediate allocations, supports boxed coercion methods and revised addition semantics, and integrates BigInt pool memory with garbage collection. Compiler constant tracking and tests cover coercion, arithmetic correctness, retention, and pool cleanup. ChangesBigInt runtime
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant gc_run
participant gc_objects
participant gc_bigints
gc_run->>gc_bigints: gc_bigints_begin(js)
gc_objects->>gc_bigints: gc_bigints_mark(payload)
gc_run->>gc_bigints: gc_bigints_sweep(js)
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/modules/bigint.c (2)
347-414: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the output-buffer capacity contract explicit.
These helpers moved from "allocate and return" to "write into a caller buffer", and the required capacity is now implicit:
bigint_add_abs_limbsalways writesresult[maxlen](needsmax(alen,blen)+1),bigint_sub_abs_limbsneedsalen, andbigint_mul_abs_limbsneedsalen+blen+1. All current call sites satisfy this, but an under-sized caller would silently overrun a 32-limb stack buffer. Also,bigint_add_u32_inplacesilently drops the carry whencount == capacityinstead of signalling — currently unreachable, but a quiet wrong-result path.Suggest documenting the minimum capacity on each helper and adding
assert()s (or passingcapacityconsistently asbigint_add_u32_inplacealready does).🤖 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 `@src/modules/bigint.c` around lines 347 - 414, Make the caller-buffer capacity contract explicit for bigint_add_abs_limbs, bigint_sub_abs_limbs, and bigint_mul_abs_limbs by documenting each helper’s minimum required capacity and asserting it at entry or otherwise validating it consistently. Update bigint_add_u32_inplace so a carry remaining when count reaches capacity is detected and signalled rather than silently discarded, while preserving existing behavior for valid-capacity callers.
1141-1182: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider extracting the "stack buffer vs. payload" scaffold.
The
stack_limbs/capacity > BIGINT_STACK_LIMBS/ allocate / re-fetch limb pointers /bigint_finish_payload-or-js_mkbigint_limbsblock is now repeated inbigint_add,bigint_sub,bigint_mul,bigint_bitwise_binary,bigint_bitnot,bigint_shift_left, andbigint_shift_right. Correct in every instance here, but the mandatory post-allocation re-fetch ofad/bd/limbsis exactly the kind of step that gets dropped in a future edit, and the failure mode is a silent use-after-GC.A small
bigint_result_buf_tholding{ ant_value_t out; bigint_payload_t *payload; uint32_t *limbs; uint32_t stack[BIGINT_STACK_LIMBS]; }withbigint_result_reserve()/bigint_result_finish()would centralize both the capacity decision and the re-fetch requirement.🤖 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 `@src/modules/bigint.c` around lines 1141 - 1182, Extract the repeated result-buffer setup and completion logic from bigint_add, bigint_sub, bigint_mul, bigint_bitwise_binary, bigint_bitnot, bigint_shift_left, and bigint_shift_right into a bigint_result_buf_t with bigint_result_reserve() and bigint_result_finish(). Centralize stack-versus-payload allocation, ensure reserve re-fetches all input limb pointers after allocation, and use the finish helper for both payload and stack-backed results while preserving each operation’s existing capacity, length, and sign behavior.examples/spec/bigint.js (1)
18-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage of the coercion rules; two gaps worth closing.
testThrows(examples/spec/helpers.jslines 64-73) accepts any thrown value, so these cases pass even though1n + 1currently throws a plainErrorrather thanTypeError(seesrc/silver/ops/arithmetic.hline 60). An error-type assertion would have caught it.- Nothing here crosses the new
BIGINT_STACK_LIMBS(32 limbs / 1024-bit) boundary insrc/modules/bigint.c, which is where this PR switches from the stack buffer to a GC-allocated payload — the highest-risk path is untested. A(1n << 2000n)-scale add/sub/mul/shift round-trip would exercise it.💚 Suggested additions
+const huge = 1n << 2000n; +test('bigint payload add', (huge + huge) === (huge * 2n), true); +test('bigint payload sub', (huge - huge) === 0n, true); +test('bigint payload shift roundtrip', ((huge << 64n) >> 64n) === huge, true);🤖 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 `@examples/spec/bigint.js` around lines 18 - 54, Strengthen the bigint tests by extending testThrows in helpers.js to optionally assert the thrown error type, then use that assertion for mixed bigint/number, boolean, null, undefined, and symbol additions to require TypeError. Add coverage in the bigint specification around BIGINT_STACK_LIMBS using values beyond 1024 bits, exercising addition, subtraction, multiplication, and shifts with round-trip result checks.
🤖 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 `@src/modules/bigint.c`:
- Around line 1696-1704: Update builtin_bigint_valueOf so the non-BigInt
receiver path throws a TypeError rather than a generic Error, matching the
behavior of the corresponding BigInt toString implementation while preserving
primitive unwrapping and successful BigInt returns.
- Around line 1651-1654: Update the non-BigInt receiver fallback in
BigInt.prototype.toString to call js_mkerr_typed with JS_ERR_TYPE instead of
js_mkerr, while preserving the existing unwrap_primitive validation and error
message.
In `@src/silver/engine.c`:
- Around line 395-407: Re-resolve the frame slot pointer after both
sv_add_to_primitive calls and before any slot write: update the builder append
path at src/silver/engine.c lines 395-407 and the snapshot path at
src/silver/engine.c lines 456-471 using sv_frame_slot_ptr(frame, slot_idx). This
ensures sv_slot_generic_add_store and feedback writes use current storage after
user-code re-entry.
In `@src/silver/ops/arithmetic.h`:
- Around line 59-60: Update the BigInt rejection paths to construct TypeError
instances via js_mkerr_typed(js, JS_ERR_TYPE, ...) instead of js_mkerr:
arithmetic.h lines 59-60 for mixed BigInt operations, bigint.c lines 1651-1654
for toString, and bigint.c lines 1696-1704 for valueOf. Preserve each existing
error message.
---
Nitpick comments:
In `@examples/spec/bigint.js`:
- Around line 18-54: Strengthen the bigint tests by extending testThrows in
helpers.js to optionally assert the thrown error type, then use that assertion
for mixed bigint/number, boolean, null, undefined, and symbol additions to
require TypeError. Add coverage in the bigint specification around
BIGINT_STACK_LIMBS using values beyond 1024 bits, exercising addition,
subtraction, multiplication, and shifts with round-trip result checks.
In `@src/modules/bigint.c`:
- Around line 347-414: Make the caller-buffer capacity contract explicit for
bigint_add_abs_limbs, bigint_sub_abs_limbs, and bigint_mul_abs_limbs by
documenting each helper’s minimum required capacity and asserting it at entry or
otherwise validating it consistently. Update bigint_add_u32_inplace so a carry
remaining when count reaches capacity is detected and signalled rather than
silently discarded, while preserving existing behavior for valid-capacity
callers.
- Around line 1141-1182: Extract the repeated result-buffer setup and completion
logic from bigint_add, bigint_sub, bigint_mul, bigint_bitwise_binary,
bigint_bitnot, bigint_shift_left, and bigint_shift_right into a
bigint_result_buf_t with bigint_result_reserve() and bigint_result_finish().
Centralize stack-versus-payload allocation, ensure reserve re-fetches all input
limb pointers after allocation, and use the finish helper for both payload and
stack-backed results while preserving each operation’s existing capacity,
length, and sign behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d07b9e66-97c3-4940-bb38-8c6b72062996
📒 Files selected for processing (11)
examples/spec/bigint.jsinclude/gc/bigints.hmeson/pgo/profiles/ant-darwin-aarch64.profdatasrc/gc/bigints.csrc/gc/gc.csrc/gc/objects.csrc/modules/bigint.csrc/silver/compiler.csrc/silver/engine.csrc/silver/ops/arithmetic.htests/test_bigint_gc.cjs
# Conflicts: # src/gc/gc.c # src/modules/bigint.c
1 participant
Summary by CodeRabbit
New Features
BigInt.prototype.valueOf()support.Bug Fixes
Tests