Conversation
📝 WalkthroughWalkthroughThis PR centralizes vector types into a generic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
38-41: Consider usingself.len()directly instead ofself.as_slice().len().Since
FqSliceMuthas alen()method through the base type,self.as_slice().len()creates an unnecessary intermediateFqSlice. This is a minor inefficiency but inconsistent with similar patterns elsewhere in the file (e.g., line 412).🔎 Proposed fix
pub fn add_offset(&mut self, other: FqSlice<'_, F>, c: FieldElement<F>, offset: usize) { - self.slice_mut(offset, self.as_slice().len()) + self.slice_mut(offset, self.len()) .add(other.slice(offset, other.len()), c) }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/fp_wrapper/mod.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/inner.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-11T08:35:31.031Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 205
File: ext/crates/fp/src/limb.rs:11-28
Timestamp: 2025-12-11T08:35:31.031Z
Learning: In Rust 2024 edition (Cargo.toml edition = 2024), the mem::size_of, size_of_val, align_of, and align_of_val functions from std::mem are in the prelude and do not require explicit imports. During reviews, these can be used unqualified when the project targets edition = 2024. Verify the crate's Cargo.toml edition setting before assuming unqualified use, and avoid adding extraneous imports for these helpers.
Applied to files:
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-14T08:23:53.399Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/simd/x86_64/avx512.rs:367-379
Timestamp: 2025-12-14T08:23:53.399Z
Learning: In Rust 2024 edition, do not remove explicit unsafe blocks inside unsafe fn bodies. The lint unsafe_op_in_unsafe_fn is enabled by default, treating the body of an unsafe fn as safe and requiring each unsafe operation to be wrapped in an explicit unsafe { ... } block. Do not suggest removing these blocks in code using edition = "2024"; ensure unsafe operations are enclosed in explicit unsafe blocks and review for unnecessary nesting.
Applied to files:
ext/crates/algebra/src/module/free_module.rsext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-15T22:15:17.779Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/benches/mul.rs:19-28
Timestamp: 2025-12-15T22:15:17.779Z
Learning: In the fp crate, matrices over F_2 (prime=2) are stored with 1 bit per matrix entry. This means an M×N matrix uses M*N bits, i.e. M*N/8 bytes. For example, an 8192×8192 F_2 matrix uses (8192*8192)/8 = 8,388,608 bytes = 8 MB. Use this packing assumption when analyzing memory usage and bench tests, and rely on the crate's API for bit-packed access rather than byte-per-entry storage.
Applied to files:
ext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
📚 Learning: 2025-12-20T04:55:22.617Z
Learnt from: JoeyBF
Repo: SpectralSequences/sseq PR: 207
File: ext/crates/fp/src/matrix/matrix_inner.rs:75-89
Timestamp: 2025-12-20T04:55:22.617Z
Learning: In reviews of code under ext/crates/fp/, treat AVec::from_iter(0, data) as using AVec's default alignment, not 0-byte alignment. Do not assume a reduced alignment or override of AVec's inherent alignment guarantees. If code relies on a specific alignment, verify it via AVec's type alias and its documentation, and avoid making changes that could alter alignment semantics. Ensure tests or comments reflect that 0 is a default alignment flag and not a zero-byte alignment.
Applied to files:
ext/crates/fp/src/vector/impl_fqslicemut.rsext/crates/fp/src/vector/impl_fqslice.rsext/crates/fp/src/vector/impl_fqvector.rsext/crates/fp/src/vector/fp_wrapper/helpers.rsext/crates/fp/src/vector/inner.rsext/crates/fp/src/matrix/matrix_inner.rsext/crates/fp/src/vector/fp_wrapper/mod.rs
🧬 Code graph analysis (5)
ext/crates/fp/src/vector/impl_fqslicemut.rs (3)
ext/crates/fp/src/vector/inner.rs (4)
fq(68-70)len(76-78)start(150-152)end(154-156)ext/crates/fp/src/vector/iter.rs (1)
len(110-112)ext/crates/fp/src/vector/impl_fqslice.rs (1)
from(42-44)
ext/crates/fp/src/vector/impl_fqslice.rs (3)
ext/crates/fp/src/vector/inner.rs (4)
start(150-152)end(154-156)len(76-78)_new(54-66)ext/crates/fp/src/vector/fp_wrapper/mod.rs (3)
from(194-196)from(253-255)from(259-261)ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
from(445-447)
ext/crates/fp/src/vector/inner.rs (5)
ext/crates/fp/src/matrix/matrix_inner.rs (8)
iter(380-382)iter(1412-1419)prime(166-168)prime(1388-1390)new(47-49)new(1209-1224)fmt(430-445)fmt(449-451)ext/crates/fp/src/field/element.rs (3)
field(45-47)new(41-43)fmt(110-112)ext/crates/algebra/src/algebra/algebra_trait.rs (1)
prime(42-42)ext/crates/fp/src/vector/impl_fqslicemut.rs (2)
new(168-202)new(288-321)ext/crates/fp/src/vector/fp_wrapper/mod.rs (1)
fmt(180-190)
ext/crates/fp/src/matrix/matrix_inner.rs (1)
ext/crates/fp/src/vector/inner.rs (4)
_new(54-66)limb_range(172-174)start(150-152)end(154-156)
ext/crates/fp/src/vector/fp_wrapper/mod.rs (2)
ext/crates/fp/src/vector/inner.rs (8)
iter(101-103)len(76-78)slice(85-94)as_slice(96-98)iter_nonzero(105-107)limbs(158-160)slice_mut(311-321)as_slice_mut(215-217)ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
from(445-447)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: webserver (beta)
- GitHub Check: test (stable)
- GitHub Check: test (beta)
- GitHub Check: webserver (nightly)
- GitHub Check: miri
- GitHub Check: lint
- GitHub Check: test (nightly)
🔇 Additional comments (16)
ext/crates/fp/src/matrix/matrix_inner.rs (2)
368-376: LGTM: Row accessor constructor updates are consistent.The switch from
FpSlice::new/FpSliceMut::newto_newaligns with the new internal constructor visibility pattern introduced in the vector base refactor. The parameters and semantics remain unchanged.
1266-1272: Appropriate use of new slicing APIs.Line 1266 correctly uses the internal
_newconstructor for mutable slice construction, while line 1272 uses the new publicrestrict()method for sub-slicing an immutable slice. This follows the intended API split between internal construction and public sub-slicing.ext/crates/fp/src/vector/fp_wrapper/helpers.rs (2)
23-41: Clean separation of read vs. write helpers using Repr traits.The split of helpers into
Repr(read-only:entry_helper) andReprMut(mutable:scale_helper,set_entry_helper,add_basis_element_helper) impl blocks correctly aligns with the trait bounds and follows the new representation abstraction pattern.
72-79: Newadd_offset_helperis consistent with existing helper pattern.The helper follows the established pattern of converting
F::ElementContainer(u32) toFieldElement<F>before delegating to the underlying method.ext/crates/fp/src/vector/impl_fqslicemut.rs (1)
442-447: Generic From implementation correctly uses ReprMut bound.The generalization from
From<&'a mut FqVector<F>>toFrom<&'a mut FqVectorBase<A, R, F>>withR: ReprMutallows conversion from any mutable vector-base representation, aligning with the unified type design.ext/crates/fp/src/vector/impl_fqvector.rs (2)
5-5: Import path updated to reflect new type alias location.
FqVector<F>is now a type alias toFqVectorBase<true, Vec<Limb>, F>defined ininner.rs, so the import correctly points there.
11-245: FqVector implementation correctly retains owned-vector-specific operations.Methods that require ownership or Vec-specific operations (construction, resize, capacity management, limb-level arithmetic) are appropriately kept here, while common operations have been moved to
FqVectorBase.ext/crates/fp/src/vector/impl_fqslice.rs (2)
11-21: Appropriate rename fromslice()torestrict().The rename to
restrict()clarifies that this method narrows an existing slice rather than creating a slice from an owned vector, avoiding confusion withFqVectorBase::slice(). The implementation correctly uses the internal_newconstructor and preserves the lifetime throughinto_limbs().
41-44: Generic From implementation correctly uses Repr bound.The generalization allows converting any
FqVectorBasewith aRepr-bounded storage type to anFqSlice, enabling uniform slice creation from vectors, slices, and other representations.ext/crates/fp/src/vector/fp_wrapper/mod.rs (3)
48-63: Well-designed enum-based wrapper with generic representation.The
FpVectorBase<const A: bool, R>enum cleanly abstracts over prime variants while theAandRparameters enable the same aligned/representation flexibility as the underlyingFqVectorBase. The type aliases provide ergonomic access to the most common configurations.
73-100: Correct trait-bounded impl blocks for read vs. write operations.The separation into
Repr(read:slice,as_slice,iter,iter_nonzero,is_zero,entry,limbs) andReprMut(write:slice_mut,as_slice_mut,set_to_zero,scale,set_entry,add_basis_element,limbs_mut) impl blocks correctly mirrors the underlyingFqVectorBasedesign.
142-148: FpSlice API aligns with FqSlice changes.The
_newconstructor (pub(crate)),restrict(replacingslice), andto_ownedmethods match the correspondingFqSliceAPI, maintaining consistency across the Fp/Fq abstraction layers.ext/crates/fp/src/vector/inner.rs (4)
18-24: Clean trait design for storage representation abstraction.The
ReprandReprMuttraits with blanket implementations overDeref<Target = [Limb]>andDerefMut<Target = [Limb]>respectively enable flexible storage strategies (Vec, slices, Cow, Arc, etc.) while maintaining a uniform interface.
53-65: Alignment validation in constructor is correct.The assertion at line 57 ensures that when
Ais true (aligned vectors), the start position is always on a limb boundary. This invariant enables the fast paths in methods likeis_zero()andset_to_zero().
176-184: Edge case documentation is appreciated.The comment explaining the potential underflow for zero-width slices at the start of an
FpVectoris helpful. The performance trade-off (avoidingsaturating_subfor a 10% gain) is reasonable given the rarity of this edge case.
339-363: Display implementation handles both formats correctly.The alternate format (
{:#}) provides compact representation suitable for prime fields < 10, while the default format provides a bracketed, comma-separated list. The implementation is consistent with theFpVectorBaseDisplay impl infp_wrapper/mod.rs.
The similarly-named but crucially different `FqSlice::slice` has been renamed to `restrict`
This solves the compilation issues when odd-primes is disabled
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ext/crates/fp/src/vector/inner.rs`:
- Around line 176-197: Add a doc comment to max_limb_mask mirroring the
underflow note in limb_range_inner: explain that computing (self.end() - 1)
underflows when self.end() == 0 (which happens for a width-0 slice at start of
an FpVector), that callers currently avoid this via limb_range.is_empty()
checks, and that handling it here would require saturating/subtraction changes
with a performance cost; reference the function name max_limb_mask and the
related limb_range_inner to make the invariant clear to future maintainers.
- Around line 18-24: Make the ReprMut relationship to Repr explicit: change the
ReprMut trait to extend Repr (e.g., declare ReprMut: Repr + DerefMut<Target =
[Limb]> ) and update its blanket impl to require the same bounds (ensure the
impl for ReprMut uses types that implement DerefMut<Target = [Limb]> and also
satisfy Repr or equivalently both Deref and DerefMut bounds). This makes the
relationship between Repr and ReprMut clear while keeping existing behavior for
the traits and the impls.
- Around line 109-131: In is_zero(), avoid re-checking the same single limb when
limb_range.len() == 1: compute limb_range and (min_mask, max_mask) as you
already do, then if limb_range.len() == 1 perform the first masked check on
self.limbs()[limb_range.start] with min_mask and return its negation (i.e.
return false if masked bits present, otherwise true) so you can early-return and
skip the later inner_range and final masked checks; keep existing behavior for
multi-limb cases using limb_range_inner(), limb_masks(), and the other checks
unchanged.
| pub trait Repr: Deref<Target = [Limb]> {} | ||
|
|
||
| /// A mutable slice of an `FqVector`. | ||
| impl<T: Deref<Target = [Limb]>> Repr for T {} | ||
|
|
||
| pub trait ReprMut: DerefMut<Target = [Limb]> {} | ||
|
|
||
| impl<T: DerefMut<Target = [Limb]>> ReprMut for T {} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider making ReprMut explicitly extend Repr for clarity.
While the current design works because DerefMut: Deref implies R: Repr via the blanket impl, making this relationship explicit would improve code clarity and self-documentation.
♻️ Suggested change
-pub trait ReprMut: DerefMut<Target = [Limb]> {}
+pub trait ReprMut: Repr + DerefMut<Target = [Limb]> {}
-impl<T: DerefMut<Target = [Limb]>> ReprMut for T {}
+impl<T: Repr + DerefMut<Target = [Limb]>> ReprMut for T {}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub trait Repr: Deref<Target = [Limb]> {} | |
| /// A mutable slice of an `FqVector`. | |
| impl<T: Deref<Target = [Limb]>> Repr for T {} | |
| pub trait ReprMut: DerefMut<Target = [Limb]> {} | |
| impl<T: DerefMut<Target = [Limb]>> ReprMut for T {} | |
| pub trait Repr: Deref<Target = [Limb]> {} | |
| impl<T: Deref<Target = [Limb]>> Repr for T {} | |
| pub trait ReprMut: Repr + DerefMut<Target = [Limb]> {} | |
| impl<T: Repr + DerefMut<Target = [Limb]>> ReprMut for T {} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ext/crates/fp/src/vector/inner.rs` around lines 18 - 24, Make the ReprMut
relationship to Repr explicit: change the ReprMut trait to extend Repr (e.g.,
declare ReprMut: Repr + DerefMut<Target = [Limb]> ) and update its blanket impl
to require the same bounds (ensure the impl for ReprMut uses types that
implement DerefMut<Target = [Limb]> and also satisfy Repr or equivalently both
Deref and DerefMut bounds). This makes the relationship between Repr and ReprMut
clear while keeping existing behavior for the traits and the impls.
| pub fn is_zero(&self) -> bool { | ||
| if A { | ||
| return self.limbs().iter().all(|&x| x == 0); | ||
| } | ||
|
|
||
| let limb_range = self.limb_range(); | ||
| if limb_range.is_empty() { | ||
| return true; | ||
| } | ||
| let (min_mask, max_mask) = self.limb_masks(); | ||
| if self.limbs()[limb_range.start] & min_mask != 0 { | ||
| return false; | ||
| } | ||
|
|
||
| let inner_range = self.limb_range_inner(); | ||
| if !inner_range.is_empty() && self.limbs()[inner_range].iter().any(|&x| x != 0) { | ||
| return false; | ||
| } | ||
| if self.limbs()[limb_range.end - 1] & max_mask != 0 { | ||
| return false; | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor redundancy when limb_range.len() == 1.
When there's only one limb spanning the slice, limb_masks() returns identical masks for both min and max. This means lines 119 and 127 check the same limb with the same mask—redundant but functionally correct. Consider an early return after the first check when limb_range.len() == 1 to avoid the duplicate work.
♻️ Possible optimization
let limb_range = self.limb_range();
if limb_range.is_empty() {
return true;
}
let (min_mask, max_mask) = self.limb_masks();
if self.limbs()[limb_range.start] & min_mask != 0 {
return false;
}
+ if limb_range.len() == 1 {
+ return true;
+ }
let inner_range = self.limb_range_inner();
if !inner_range.is_empty() && self.limbs()[inner_range].iter().any(|&x| x != 0) {
return false;
}
- if self.limbs()[limb_range.end - 1] & max_mask != 0 {
- return false;
- }
- true
+ self.limbs()[limb_range.end - 1] & max_mask == 0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn is_zero(&self) -> bool { | |
| if A { | |
| return self.limbs().iter().all(|&x| x == 0); | |
| } | |
| let limb_range = self.limb_range(); | |
| if limb_range.is_empty() { | |
| return true; | |
| } | |
| let (min_mask, max_mask) = self.limb_masks(); | |
| if self.limbs()[limb_range.start] & min_mask != 0 { | |
| return false; | |
| } | |
| let inner_range = self.limb_range_inner(); | |
| if !inner_range.is_empty() && self.limbs()[inner_range].iter().any(|&x| x != 0) { | |
| return false; | |
| } | |
| if self.limbs()[limb_range.end - 1] & max_mask != 0 { | |
| return false; | |
| } | |
| true | |
| } | |
| pub fn is_zero(&self) -> bool { | |
| if A { | |
| return self.limbs().iter().all(|&x| x == 0); | |
| } | |
| let limb_range = self.limb_range(); | |
| if limb_range.is_empty() { | |
| return true; | |
| } | |
| let (min_mask, max_mask) = self.limb_masks(); | |
| if self.limbs()[limb_range.start] & min_mask != 0 { | |
| return false; | |
| } | |
| if limb_range.len() == 1 { | |
| return true; | |
| } | |
| let inner_range = self.limb_range_inner(); | |
| if !inner_range.is_empty() && self.limbs()[inner_range].iter().any(|&x| x != 0) { | |
| return false; | |
| } | |
| self.limbs()[limb_range.end - 1] & max_mask == 0 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ext/crates/fp/src/vector/inner.rs` around lines 109 - 131, In is_zero(),
avoid re-checking the same single limb when limb_range.len() == 1: compute
limb_range and (min_mask, max_mask) as you already do, then if limb_range.len()
== 1 perform the first masked check on self.limbs()[limb_range.start] with
min_mask and return its negation (i.e. return false if masked bits present,
otherwise true) so you can early-return and skip the later inner_range and final
masked checks; keep existing behavior for multi-limb cases using
limb_range_inner(), limb_masks(), and the other checks unchanged.
| /// This function underflows if `self.end() == 0`, which happens if and only if we are taking a | ||
| /// slice of width 0 at the start of an `FpVector`. This should be a very rare edge case. | ||
| /// Dealing with the underflow properly would probably require using `saturating_sub` or | ||
| /// something of that nature, and that has a nontrivial (10%) performance hit. | ||
| #[inline] | ||
| pub(super) fn limb_range_inner(&self) -> Range<usize> { | ||
| let range = self.limb_range(); | ||
| (range.start + 1)..(usize::max(range.start + 1, range.end - 1)) | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub(super) fn min_limb_mask(&self) -> Limb { | ||
| !0 << self.offset() | ||
| } | ||
|
|
||
| #[inline(always)] | ||
| pub(super) fn max_limb_mask(&self) -> Limb { | ||
| let num_entries = 1 + (self.end() - 1) % self.fq().entries_per_limb(); | ||
| let bit_max = num_entries * self.fq().bit_length(); | ||
|
|
||
| (!0) >> (crate::constants::BITS_PER_LIMB - bit_max) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the underflow risk in max_limb_mask similar to limb_range_inner.
Line 193 has the same underflow risk when self.end() == 0 as limb_range_inner (documented at lines 176-179). While callers currently guard against this via limb_range.is_empty() checks, adding a similar doc comment to max_limb_mask would help future maintainers understand the invariant.
📝 Suggested documentation
#[inline(always)]
pub(super) fn max_limb_mask(&self) -> Limb {
+ // NB: This underflows if `self.end() == 0`. Callers must guard against empty ranges.
let num_entries = 1 + (self.end() - 1) % self.fq().entries_per_limb();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ext/crates/fp/src/vector/inner.rs` around lines 176 - 197, Add a doc comment
to max_limb_mask mirroring the underflow note in limb_range_inner: explain that
computing (self.end() - 1) underflows when self.end() == 0 (which happens for a
width-0 slice at start of an FpVector), that callers currently avoid this via
limb_range.is_empty() checks, and that handling it here would require
saturating/subtraction changes with a performance cost; reference the function
name max_limb_mask and the related limb_range_inner to make the invariant clear
to future maintainers.
1 participant
This PR abstracts away the common logic behind
FqVector,FqSliceandFqSliceMut. This leads to simplifications and the possibility of adding more variants in the future.Currently, my main objective is switching to using
FpCowinBidegreeElements to avoid unnecessary clones and make them usable in performance-critical code.Summary by CodeRabbit
Refactor