Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the lowest_common_ancestor library to use the shared doubling crate for binary lifting, replacing its bespoke ancestor table implementation.
Changes:
- Replace
ancestortable inLowestCommonAncestorwithDoubling<()>-based transitions. - Add
doublingas a dependency of thelowest_common_ancestorcrate. - Add a
Valuetrait implementation for()in thedoublingcrate to support value-less transitions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| libs/lowest_common_ancestor/src/lib.rs | Replaces manual binary-lifting table with Doubling<()> and sentinel state logic. |
| libs/lowest_common_ancestor/Cargo.toml | Adds the local doubling crate as a dependency. |
| libs/doubling/src/lib.rs | Implements Value for () to allow Doubling<()> usage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let sentinel = n; | ||
| let doubling = Doubling::new(n + 1, n, |i| { | ||
| if i < n { | ||
| let next = parent[i].unwrap_or(sentinel); | ||
| Transition::new(next, ()) | ||
| } else { | ||
| Transition::new(sentinel, ()) | ||
| } | ||
| }); |
There was a problem hiding this comment.
Doubling::new is instantiated with max_steps = n, but the maximum number of upward steps needed in an n-node tree is n-1 (max depth). With max_steps = n, Doubling builds one extra level when n is a power of two (since log2_max_steps = floor(log2(n))), increasing memory/time for large n while never being queried. Consider using max_steps = n.saturating_sub(1).max(1) (or depth.iter().max() if you want the exact bound) to avoid the extra table row while keeping the n == 1 case valid.
| } | ||
|
|
||
| impl Value for () { | ||
| fn op(&self, _other: &Self) -> Self {} |
There was a problem hiding this comment.
The Value for () implementation relies on an empty block to return unit. This compiles, but it’s easy to misread as an unfinished implementation and can be flagged by style/lint tooling. Prefer returning () explicitly (and you can also omit the unused self name by using _ if desired) to make the intent clear.
| fn op(&self, _other: &Self) -> Self {} | |
| fn op(&self, _other: &Self) -> Self { | |
| () | |
| } |
1 participant
No description provided.