bigint: centralize sign access - #1699

Reviewing this even though it's a draft — Alexander asked for drafts to be covered too.

The refactor is clean. I checked all 11 header reads across the 10 sites against the originals, receiver by receiver, since a mechanical eight-file change is exactly where self and rhs get swapped — add.rs:8 is rhs.sign() where the original read *rhs.0.header(), sub.rs:8 keeps .flip() on rhs, mul.rs:37 compares self against rhs. No slip. Read counts are unchanged everywhere too: add_signed and cmp still bind once to a local, mul still reads twice, so nothing turned a cached read into repeated calls. Nothing here has interior mutability, so repeats couldn't have diverged anyway.

The one expression that isn't literally identical is mul.rs, which went from &Sign == &Sign (auto-deref through PartialEq for &T) to Sign == Sign. For a Copy fieldless enum with derived PartialEq that's the same comparison. neg.rs is the only borrow-order-sensitive site, and sign() returns an owned Sign so the borrow ends before index_iter().

Rather than reason about it further I built both commits and ran a differential. Important detail: PartialEq for BigInt compares items and ignores sign, so equality would have hidden exactly the regression worth worrying about — I dumped Debug instead. Corpus of 28 values (±0 plus both signs of 13 magnitudes: 1, 2, 5, 42, u64::MAX, u64::MAX-1, [0,1], [1,1], [MAX,MAX], [0,0,1], [MAX,0,1], a 3-limb mixed value, [MAX;4]), running add/sub/mul/cmp/== over all 28×28 pairs plus per-value Debug, neg, and shifts at 0, 1, 5, 63, 64, 65, 127, 128, 200, 1000, a multi-word shift and a negative amount, each wrapped in catch_unwind so a panic difference would surface:

4648 cases, dumps identical, 0 panics either side

Zero, negative zero, neg(0), mul by zero, cross-sign cmp and shift-by-zero are all in there.

Gates: cargo fmt -- --check clean, cargo clippy clean, cargo test 110 + 1 + 5 doctests all passing, and a forced cargo build is warning-free — which is the direct proof that the seven removed IContainer imports were genuinely unused. npm test 3272/0, tsc --noEmit clean.

Two things worth knowing:

• cargo clippy --all-targets fails, on assert_eq!(x, x) at nanvm-lib/src/vm/mod.rs:40 (clippy::eq_op, deny-by-default). I ran it at the merge-base and got the identical failure, so it's pre-existing, not yours. Plain cargo clippy — what the body claims — is clean on both.
• On this machine every cargo command aborts because ~/.cargo/config.toml sets build.rustc = "rust-fv-driver" and that binary can't load librustc_driver. I ran the gates with CARGO_BUILD_RUSTC=rustc. Unrelated to the PR, but CI-equivalent runs here need the override until that driver is rebuilt.

The deleted todo is genuinely resolved — both its tasks are done, grep finds no dangling reference to sign-accessor, and the sibling sign-algebra.md refers to symbols rather than linking the file. Centralization is complete: the only remaining header() reads are the new accessor itself, generic container equality, and Function's own accessors — which is the pattern this copies. The IContainer imports still present in bigint/ are all genuinely used (debug.rs keeps it for items(), not the header). Making sign() plain private rather than the pub(crate) the todo proposed is strictly narrower and still reaches the child modules; not a finding.

One thing to fix before this leaves draft: the Changelog: placement, the same double violation as three sibling PRs this week. - Changelog: none is the third bullet inside ### Description rather than a section, and ### Testing follows it before the [Codex Task] trailer; CONTRIBUTING.md:186-202 wants a top-level Changelog: section, last before the trailer.

The content none is right, though not for the reason one might assume — changelog/ is not fjs-only (0.46.0/1579.md and 0.37.0.md both carry nanvm-lib entries), so nanvm-lib is in scope. It's right because changelog/README.md limits entries to behaviour or public-API changes and exempts internal refactors: this adds one private method, changes no public signature, and I proved behaviour identical over 4648 cases.

Nothing else reads as unfinished, so as far as I can tell the body format is the only thing between this and ready-for-review.

Not verified: clippy under the repo's default rustc wrapper (see above), npm run update (deno is absent here too), and non-Naive VM backends — sign() is generic over A: IVm and forwards to the same header() call, so divergence isn't plausible, but it isn't empirically covered.

Pre-existing and unreachable through the public API, so not a finding here: cmp(-0, +0) is Less while -0 == +0 is true — identical on base and head.

Pull request overview

This PR refactors nanvm-lib’s BigInt implementation to centralize sign retrieval behind a single BigInt::sign() helper, eliminating repeated direct container-header reads across bigint operator modules and reducing fragile IContainer-only imports.

Changes:

• Added BigInt::sign(&self) -> Sign and routed internal sign reads through it.
• Updated bigint operator/utility modules (add, sub, mul, neg, shl, shr, cmp, debug) to use self.sign() / rhs.sign() instead of *self.0.header().
• Removed the resolved nanvm-lib/todo/bigint-sign-accessor.md todo entry.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.