Add BitsPrecision: runtime cross-carrier width (0.2.1) - #15
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds public ChangesBits precision API
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Consider adding a short, explicit mathematical definition for
bit-length(e.g.,bit_length(x) = 0 if x = 0, otherwise floor(log2(x)) + 1) to remove any ambiguity about off-by-one behavior or signed vs unsigned interpretation. - Where you describe trait surfaces (e.g.,
bit_widthandbit_lengthas runtimefns and::BITSas a const), it may help to include a tiny pseudo-signature block showing which traits are expected to expose which functions/consts, to make implementor expectations unambiguous.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider adding a short, explicit mathematical definition for `bit-length` (e.g., `bit_length(x) = 0 if x = 0, otherwise floor(log2(x)) + 1`) to remove any ambiguity about off-by-one behavior or signed vs unsigned interpretation.
- Where you describe trait surfaces (e.g., `bit_width` and `bit_length` as runtime `fn`s and `::BITS` as a const), it may help to include a tiny pseudo-signature block showing which traits are expected to expose which functions/consts, to make implementor expectations unambiguous.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces notes/BIT_VOCABULARY.md to establish a canonical vocabulary for bit quantities (capacity, width, and bit-length) across the const-num-traits ecosystem. The review feedback highlights two important issues: first, the definition of bit-length as "the index of the top set bit" is mathematically inconsistent with the provided examples (e.g., the value 3 has a bit-length of 2, but the 0-based index of its top set bit is 1); second, there is a naming conflict between the suggested identifier bit_width (intended for logical width) and the existing BitWidth::bit_width trait, which currently represents bit-length.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bbef31a1a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
kaidokert
changed the title
Adds the standalone `BitsPrecision { fn bits_precision(self) -> u32 }` trait as
the cross-carrier 'width' primitive: every carrier reports its operating width at
runtime — a fixed carrier returns its type width (`u32` -> 32), a variable-width
bignum returns its constructed length. Deliberately a runtime fn with NO
associated `const BITS`: a type-level width const only works for fixed carriers
and is exactly the shape that produced the `size_of*8` / `count_zeros(zero())`
proxies. Bit-length already ships as `BitWidth::bit_width`. Additive.
8a503a0 to
05595da
Compare
kaidokert
changed the title
Drop the proxy narration and the 'this trait replaces' aside; keep the cross-carrier semantics, the runtime-not-const reason, and the BitWidth disambiguation.
![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
🤖 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/ops/bits.rs`:
- Around line 444-447: Update the documentation above the BitsPrecision trait to
clarify that its width accessor is a runtime method rather than an associated
const, while noting that it remains const-evaluable through the nightly
c0nst::c0nst! path; avoid implying that it can never be evaluated at compile
time.
🪄 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
Run ID: 91a4f7ef-e2e1-4d74-822a-c766fab6c45a
📒 Files selected for processing (4)
Cargo.tomlsrc/lib.rssrc/ops/bits.rstests/const_nightly.rs
Reword the doc: it's const-callable on nightly (there's a const canary), so 'not a const' was too strong. It's not an associated const *item*; a variable carrier's width is only known at runtime.
Replaces the WIP ZeroPrecision (self-as-witness, never released) with a
coherent width-establishment trait, the constructive companion to
BitsPrecision (which reads the width).
Why: on a runtime-width carrier, Zero::zero()/One::one() are minimal-width,
so generic code that seeds an accumulator from zero() and drives it toward a
modulus width operates at the seed width and wraps early — correct on a
fixed-width type, silently truncated on a variable-width one. Seeding at the
modulus width (T::zero_with_precision_of(&m)) removes the trap.
Shape: required widen_to_precision(self, bits) — grow-only, value-preserving,
identity on fixed-width carriers. Defaulted zero/one_with_precision(bits) and
the witness-based *_of(&witness) ergonomics. Method names mirror crypto-bigint
BoxedUint::{zero_with_precision, one_with_precision, widen}; supertrait
WithPrecision: BitsPrecision fences it to types that carry a binary width.
BitsPrecision doc reframed from "integer/bignum width" to "binary operating
width <= storage capacity" — it also covers sub-capacity fixed carriers
(arbitrary-int u48, wasteful fixed-point), and excludes decimal/float/rational
(non-binary precision). Adds the witness caveat: bits_precision(zero()) is 0 on
a runtime carrier, so probe a full-width witness, never the identity.
The `_of` witness forms (widen/zero/one_with_precision_of) carried a `Self: Copy` bound, which excluded the Clone-generic carriers they exist to serve — including ed25519's sha512_modq (the origin of the width-seed footgun), which had to keep the wrapping_sub(q, q) idiom rather than adopt zero_with_precision_of(&q). Root cause: BitsPrecision::bits_precision took `self` by value, so reading a `&Self` witness's width required copying it. bits_precision is a pure query — it reads the width and returns u32, never consuming the value — so by the crate's own FromBytes carve-out (queries that don't consume stay borrowed) it should take `&self`. Over-swept into by-value by the operand sweep. Change bits_precision to `&self` and drop every `Copy` bound from the witness forms. A non-Copy carrier now seeds at a borrowed witness's width without a clone; the witness survives the call. Proven by with_precision_serves_non_copy_ carrier (a Clone-not-Copy RtWidth). UFCS call sites take `&` (method-call syntax is source-compatible via autoref).
Compress the two trait docs (~26→~14 and ~25→~14 lines) — keep the load-bearing WHYs (the minimal-width identity footgun, the &self query rationale, the representation-compatible widening contract), drop the belaboured examples and the duplicated Copy-free explanation. Also drop two sibling-repo references from the test comments (`ed25519 sha512_modq`) — unresolvable for anyone cloning only this crate; the tests are self-explanatory.
|
/gemini review |
|
@sourcery-ai review |
|
Sorry @kaidokert, you have reached your weekly rate limit of 1500000 diff characters. Please try again later or upgrade to continue using Sourcery |
There was a problem hiding this comment.
Code Review
This pull request introduces the BitsPrecision and WithPrecision traits to const-num-traits, allowing users to query and establish a value's operating bit width, which is particularly useful for variable-width carriers. These traits are implemented for unsigned primitive integers, exported, and thoroughly tested. The reviewer suggests extending these implementations to signed primitive integers (isize, i8, i16, i32, i64, i128) to ensure completeness and consistency across the library.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Width is a value-independent property of the type (`<$t>::BITS`), well-defined for signed integers exactly as for unsigned — unlike BitWidth (bit-length), which is unsigned-only because `leading_zeros`-based length is ambiguous under a sign bit. Extends both impl macros to isize/i8..i128, matching the signed+unsigned coverage of the module's other Tier-A atoms (UnboundedShl, ShlExact) so width-bounded generic code accepts signed carriers.
1 participant
Adds the
BitsPrecision { fn bits_precision(self) -> u32 }trait — the cross-carrier width primitive.What
BitsPrecision(src/ops/bits.rs): every carrier reports its operating width at runtime. A fixed carrier returns its type width (u32→ 32,FixedUInt<T,N>→N·word); a variable-width bignum returns its constructed length (len·word). Impls foru8–u128+usize;c0nst-wrapped (const-callable on nightly, canary added).const BITS. A type-level width const only works for fixed carriers and is the exact shape that produced thesize_of*8/count_zeros(zero())proxies this replaces. Width is a runtimefnfor everyone;core::u32::BITSstays a primitive detail, not the width surface.BitWidth::bit_width(significant bits); consumers rebind thebits_precision − leading_zerosidiom onto it.Semver
Additive (new trait + impls);
0.2.0→0.2.1. Verified: stable lib + doctests, nightly const canary, clippy (default + ct), fmt.Downstream: modmath repoints
type_bit_width→operand.bits_precision(); rsa repointsbits_precision→ the trait fn andbits()→BitWidth::bit_width().Summary by CodeRabbit
BitsPrecisionto retrieve a value/type’s fixed bit precision.WithPrecisionAPIs for precision-aware zero/one creation and widening (including witness-based helpers).0.2.1.