Ensure &Node<T> is not removed - #198
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #198 +/- ##
=======================================
- Coverage 97.5% 96.7% -0.8%
=======================================
Files 10 10
Lines 1127 1216 +89
=======================================
+ Hits 1099 1177 +78
- Misses 28 39 +11 🚀 New features to boost your workflow:
|
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Good API improvement that addresses the inconsistency in #195. A few suggestions below.
Co-Authored-By: Sascha Grunert <sgrunert@redhat.com>
4823680 to
d0a082e
Compare
|
Thanks for the thorough review @saschagrunert! |
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
Second pass review, five inline comments.
| @@ -42,28 +42,20 @@ pub struct Node<T> { | |||
|
|
|||
| impl<T> Node<T> { | |||
| /// Returns a reference to the node data. | |||
There was a problem hiding this comment.
The # Panics doc section was removed, but this method still panics when called on a freed node. Since the goal of this PR is that users never get a &Node<T> to a removed node, the panic should be unreachable in practice, but it could still be hit if someone holds a &Node<T> across a mutation that frees it.
Suggestion: either keep the # Panics section so the contract is discoverable, or change the return type to Option<&T> to make it truly safe. Same applies to get_mut() below.
| pub struct Arena<T> { | ||
| nodes: Vec<Node<T>>, | ||
| pub(crate) nodes: Vec<Node<T>>, | ||
| first_free_slot: Option<usize>, |
There was a problem hiding this comment.
Widening visibility to pub(crate) gives every module direct access to the raw vec, bypassing the removed-node filtering this PR introduces. Internal code can accidentally index a removed node without the compiler catching it.
Consider a pub(crate) fn get_node_unchecked(&self, id: NodeId) -> &Node<T> helper instead. That documents the intent ("I know this might be removed") while keeping the field private.
| #[inline] | ||
| pub fn is_empty(&self) -> bool { | ||
| self.count() == 0 | ||
| self.nodes.is_empty() |
There was a problem hiding this comment.
is_empty() now checks the backing storage while count() only counts live nodes. After removing all nodes, count() == 0 is true but is_empty() is false. This inconsistency will likely surprise users.
Either align is_empty() with count() (e.g. self.iter().next().is_none()), or document that is_empty tracks storage occupancy, not live node count.
| traverser.next(); | ||
| { | ||
| let data = self.arena[*self.id].get(); | ||
| if let Some(data) = self.arena.get(*self.id).map(Node::get) { |
There was a problem hiding this comment.
If the root node is removed, this if let silently produces empty output with no indication anything was wrong. Could be confusing for someone debugging a tree.
Consider returning Err or printing a placeholder like <removed> so the caller knows the root was invalid.
2 participants
This pull request modifies the design of the API so that an
&Node<T>to a consumer will never be a removed node.Resolves #195