Skip to content

Conversation

@JoeyBF
Copy link
Collaborator

@JoeyBF JoeyBF commented Dec 23, 2025

Summary by CodeRabbit

  • New Features

    • Added mutable getter functionality to enable in-place modification of stored values.
  • Tests

    • Expanded test coverage to validate mutable access patterns and synchronization with reference data structures.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 23, 2025

Walkthrough

Added mutable accessor methods to the KdTrie multi-indexed data structure, enabling in-place mutation of stored values. The changes introduce get_mut at the public API level (KdTrie and MultiIndexed), with underlying unsafe helper methods get_child_mut and get_value_mut in the node layer, plus expanded test coverage for mutable access patterns.

Changes

Cohort / File(s) Summary
KdTrie mutable accessor
ext/crates/once/src/multiindexed/kdtrie.rs
Added public method get_mut(&mut self, coords: &[i32]) -> Option<&mut V> that traverses the trie and returns a mutable reference to the value at the given coordinates
MultiIndexed API and tests
ext/crates/once/src/multiindexed/mod.rs
Added public method get_mut(&mut self, coords: [i32; K]) -> Option<&mut V> delegating to the inner KdTrie; expanded tests with new GetMut and Modify operations to validate mutable access semantics against a reference HashMap
Node-level mutable getters
ext/crates/once/src/multiindexed/node.rs
Added unsafe pub(super) methods get_child_mut(&mut self, idx: i32) -> Option<&mut Self> and get_value_mut(&mut self, idx: i32) -> Option<&mut V> to retrieve mutable references to child nodes and leaf values

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~22 minutes

Poem

🐰 A trie now bends to mutable paws,
Where values shift without pause,
No copy, just change in place—
A rabbit's efficient grace! 🌿✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly summarizes the main change: adding mutable accessor methods (get_mut) to MultiIndexed, KdTrie, and Node types.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 42b92bf and c06975d.

📒 Files selected for processing (3)
  • ext/crates/once/src/multiindexed/kdtrie.rs
  • ext/crates/once/src/multiindexed/mod.rs
  • ext/crates/once/src/multiindexed/node.rs
🧰 Additional context used
🧠 Learnings (2)
📚 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/once/src/multiindexed/mod.rs
  • ext/crates/once/src/multiindexed/kdtrie.rs
  • ext/crates/once/src/multiindexed/node.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/once/src/multiindexed/mod.rs
  • ext/crates/once/src/multiindexed/kdtrie.rs
  • ext/crates/once/src/multiindexed/node.rs
🧬 Code graph analysis (2)
ext/crates/once/src/multiindexed/mod.rs (1)
ext/crates/once/src/multiindexed/kdtrie.rs (2)
  • get_mut (195-205)
  • get (71-82)
ext/crates/once/src/multiindexed/kdtrie.rs (3)
ext/crates/once/src/multiindexed/mod.rs (1)
  • get_mut (185-187)
ext/crates/once/src/grove/mod.rs (2)
  • get_mut (229-232)
  • get_mut (597-603)
ext/crates/once/src/once.rs (1)
  • get_mut (208-210)
⏰ 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: test (beta)
  • GitHub Check: test (nightly)
  • GitHub Check: test (stable)
  • GitHub Check: webserver (beta)
  • GitHub Check: webserver (nightly)
  • GitHub Check: miri
  • GitHub Check: lint
🔇 Additional comments (7)
ext/crates/once/src/multiindexed/node.rs (2)

111-127: LGTM: Mutable accessor for inner nodes correctly implemented.

The get_child_mut method properly mirrors the existing get_child pattern, delegating to the underlying TwoEndedGrove::get_mut. The safety documentation is clear, and the explicit unsafe block is correctly used per Rust 2024 edition requirements.


147-163: LGTM: Mutable accessor for leaf values correctly implemented.

The get_value_mut method properly mirrors the existing get_value pattern, delegating to the underlying TwoEndedGrove::get_mut. The implementation and documentation are consistent with the codebase conventions.

ext/crates/once/src/multiindexed/kdtrie.rs (1)

160-205: LGTM: get_mut implementation is correct and well-documented.

The method properly implements mutable traversal through the trie structure:

  • Correctly validates coordinate length (line 196)
  • Uses mutable rebinding pattern (let mut node = &mut self.root) without creating overlapping borrows
  • Traversal logic exactly mirrors the immutable get method
  • Documentation clearly explains the exclusive access requirement and includes helpful examples
  • Properly uses explicit unsafe blocks per Rust 2024 edition requirements

The ? operator on line 201 correctly short-circuits if any intermediate node doesn't exist, maintaining consistency with the immutable version.

ext/crates/once/src/multiindexed/mod.rs (4)

153-187: LGTM: Public API for mutable access is well-designed.

The get_mut method correctly:

  • Delegates to the underlying KdTrie::get_mut implementation
  • Requires exclusive access via &mut self, preventing concurrent access
  • Provides clear documentation with a practical example demonstrating in-place mutation of a Vec

The API design aligns well with Rust conventions and the existing immutable get method.


364-390: LGTM: Basic mutable access test is comprehensive.

The test thoroughly exercises the get_mut functionality:

  • Tests various mutation patterns (assignment, compound assignment, multiplication)
  • Covers positive and negative coordinates
  • Verifies that get_mut returns None for non-existent coordinates
  • Validates that mutations are visible through subsequent get calls

544-566: LGTM: Property test operations extended correctly.

The new operation variants are well-designed:

  • GetMut: Tests read-only mutable access
  • Modify: Tests in-place mutation by adding a delta value

The operation_strategy correctly generates all four operation types using prop_oneof!, with proper boxing for each variant.


584-633: LGTM: Property test implementation handles mutable operations correctly.

The updated test logic properly validates the new mutable access patterns:

  • Line 585: arr is correctly made mutable to support get_mut calls
  • Lines 608-613: GetMut handler dereferences to compare values correctly (&*v)
  • Lines 614-625: Modify handler updates both the array and reference HashMap, then verifies consistency
  • Lines 617, 621: wrapping_add is appropriate to prevent overflow panics in fuzz testing
  • Lines 629-633: Final verification ensures complete consistency between implementations

The test design effectively validates that mutable access maintains correctness and synchronization with the reference HashMap.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant