Skip to content

Ensure &Node<T> is not removed #198

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
russellbanks wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Ensure &Node<T> is not removed #198

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4d3bc7a
Ensure `&Node<T>` is not removed
russellbanks May 7, 2026
d0a082e
Apply suggestions from code review
russellbanks May 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 53 additions & 81 deletions indextree/src/arena.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,7 @@
use alloc::vec::Vec;

#[cfg(not(feature = "std"))]
use core::{
mem,
num::NonZeroUsize,
ops::{Index, IndexMut},
slice,
};
use core::{mem, num::NonZeroUsize};

#[cfg(feature = "par_iter")]
use rayon::prelude::*;
Expand All @@ -22,20 +17,15 @@ use rayon::prelude::*;
use serde::{Deserialize, Serialize};

#[cfg(feature = "std")]
use std::{
mem,
num::NonZeroUsize,
ops::{Index, IndexMut},
slice,
};
use std::{mem, num::NonZeroUsize};

use crate::{Node, NodeId, node::NodeData};

#[derive(PartialEq, Eq, Clone, Debug)]
#[cfg_attr(feature = "deser", derive(Deserialize, Serialize))]
/// An `Arena` structure containing certain [`Node`]s.
pub struct Arena<T> {
nodes: Vec<Node<T>>,
pub(crate) nodes: Vec<Node<T>>,
first_free_slot: Option<usize>,
last_free_slot: Option<usize>,
}
Expand Down Expand Up @@ -84,7 +74,7 @@ impl<T> Arena<T> {
/// let node = arena.get(foo).unwrap();
///
/// let node_id = arena.get_node_id(node).unwrap();
/// assert_eq!(*arena[node_id].get(), "foo");
/// assert_eq!(arena.get(node_id).map(|node| *node.get()), Some("foo"));
/// ```
pub fn get_node_id(&self, node: &Node<T>) -> Option<NodeId> {
let nodes_range = self.nodes.as_ptr_range();
Expand Down Expand Up @@ -128,7 +118,7 @@ impl<T> Arena<T> {
let index0 = index.get() - 1; // we use 1 based indexing.
self.nodes
.get(index0)
.filter(|n| !n.is_removed())
.filter(|node| !node.is_removed())
.map(|node| NodeId::from_non_zero_usize(index, node.stamp))
}

Expand All @@ -141,11 +131,11 @@ impl<T> Arena<T> {
/// # Examples
///
/// ```
/// # use indextree::Arena;
/// # use indextree::{Arena, Node};
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
///
/// assert_eq!(*arena[foo].get(), "foo");
/// assert_eq!(arena.get(foo).map(Node::get), Some(&"foo"));
/// ```
pub fn new_node(&mut self, data: T) -> NodeId {
let (index, stamp) = if let Some(index) = self.pop_front_free_node() {
Expand All @@ -167,11 +157,9 @@ impl<T> Arena<T> {
/// Returns the number of nodes in the arena, including removed nodes.
///
/// Removed nodes are still counted because they remain in the
/// internal storage. Use [`iter()`] with [`Node::is_removed()`]
/// to count only live nodes.
/// internal storage. Use [`count()`] to count only live nodes.
///
/// [`iter()`]: Arena::iter
/// [`Node::is_removed()`]: crate::Node::is_removed
/// [`count()`]: Arena::count
///
/// # Examples
///
Expand All @@ -180,17 +168,36 @@ impl<T> Arena<T> {
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
/// let _bar = arena.new_node("bar");
/// assert_eq!(arena.count(), 2);
/// assert_eq!(arena.len(), 2);
///
/// foo.remove(&mut arena);
/// // The removed node is still counted.
/// assert_eq!(arena.len(), 2);
/// ```
#[inline]
pub fn len(&self) -> usize {
self.nodes.len()
}

/// Returns the number of non-removed nodes in the arena.
///
/// # Examples
///
/// ```
/// # use indextree::Arena;
/// let mut arena = Arena::new();
/// let foo = arena.new_node("foo");
/// let _bar = arena.new_node("bar");
/// assert_eq!(arena.count(), 2);
///
/// foo.remove(&mut arena);
/// assert_eq!(arena.count(), 1);
/// ```
pub fn count(&self) -> usize {
self.nodes.len()
self.iter().count()
Copy link
Copy Markdown
Owner

@saschagrunert saschagrunert Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes count() from O(1) to O(n) since iter() now filters removed nodes. Worth a doc note about the linear cost, since the old count() was just Vec::len().

}

/// Returns `true` if arena has no nodes, `false` otherwise.
/// Returns `true` if the arena has no nodes, `false` otherwise.
///
/// # Examples
///
Expand All @@ -205,13 +212,14 @@ impl<T> Arena<T> {
/// foo.remove(&mut arena);
/// assert!(!arena.is_empty());
/// ```
#[inline]
pub fn is_empty(&self) -> bool {
self.count() == 0
self.nodes.is_empty()
}

/// Returns a reference to the node with the given id if in the arena.
///
/// Returns `None` if not available.
/// Returns `None` if the node does not exist or has been removed.
///
/// # Examples
///
Expand All @@ -238,13 +246,15 @@ impl<T> Arena<T> {
/// assert!(another_arena.get(bar).is_none());
/// ```
pub fn get(&self, id: NodeId) -> Option<&Node<T>> {
self.nodes.get(id.index0())
self.nodes
.get(id.index0())
.filter(|node| !node.is_removed())
}

/// Returns a mutable reference to the node with the given id if in the
/// arena.
///
/// Returns `None` if not available.
/// Returns `None` if the node does not exist or has been removed.
///
/// # Examples
///
Expand All @@ -258,14 +268,13 @@ impl<T> Arena<T> {
/// assert_eq!(arena.get(foo).map(|node| *node.get()), Some("FOO!"));
/// ```
pub fn get_mut(&mut self, id: NodeId) -> Option<&mut Node<T>> {
self.nodes.get_mut(id.index0())
self.nodes
.get_mut(id.index0())
.filter(|node| !node.is_removed())
}

/// Returns an iterator of all nodes in the arena in storage-order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Examples
///
/// ```
Expand All @@ -288,21 +297,17 @@ impl<T> Arena<T> {
/// bar.remove(&mut arena);
///
/// let mut iter = arena.iter();
/// assert_eq!(iter.next().map(|node| (*node.get(), node.is_removed())), Some(("foo", false)));
/// assert_eq!(iter.next().map_or(false, |node| node.is_removed()), true);
/// assert_eq!(iter.next().map(|node| (*node.get(), node.is_removed())), None);
/// assert_eq!(iter.next().map(|node| *node.get()), Some("foo"));
/// assert_eq!(iter.next(), None);
/// ```
///
/// [`is_removed()`]: Node::is_removed
pub fn iter(&self) -> slice::Iter<'_, Node<T>> {
self.nodes.iter()
pub fn iter(&self) -> impl Iterator<Item = &Node<T>> {
self.nodes.iter().filter(|node| !node.is_removed())
}

/// Returns an iterator of [`NodeId`]s of all non-removed nodes in
/// the arena in storage-order.
///
/// Unlike [`iter()`], this skips removed nodes and yields `NodeId`s
/// instead of `&Node<T>`.
/// Unlike [`iter()`], yields `NodeId`s instead of `&Node<T>`.
///
/// # Examples
///
Expand Down Expand Up @@ -331,9 +336,6 @@ impl<T> Arena<T> {

/// Returns a mutable iterator of all nodes in the arena in storage-order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Example
///
/// ```
Expand All @@ -351,9 +353,8 @@ impl<T> Arena<T> {
/// let node_refs = arena.iter().map(|i| i.get().clone()).collect::<Vec<_>>();
/// assert_eq!(node_refs, vec![5, 6]);
/// ```
/// [`is_removed()`]: Node::is_removed
pub fn iter_mut(&mut self) -> slice::IterMut<'_, Node<T>> {
self.nodes.iter_mut()
pub fn iter_mut(&mut self) -> impl Iterator<Item = &mut Node<T>> {
self.nodes.iter_mut().filter(|node| !node.is_removed())
}

/// Clears all the nodes in the arena, but retains its allocated capacity.
Expand All @@ -372,22 +373,12 @@ impl<T> Arena<T> {
self.last_free_slot = None;
}

/// Returns a slice of the inner nodes collection.
///
/// The slice contains all nodes in storage order, including removed
/// nodes. Use [`Node::is_removed()`] to filter them out.
///
/// [`Node::is_removed()`]: crate::Node::is_removed
pub fn as_slice(&self) -> &[Node<T>] {
self.nodes.as_slice()
}

pub(crate) fn free_node(&mut self, id: NodeId) {
let node = &mut self[id];
let node = &mut self.nodes[id.index0()];
node.data = NodeData::NextFree(None);
node.stamp.as_removed();
let stamp = node.stamp;
if stamp.reuseable() {
if stamp.reusable() {
if let Some(index) = self.last_free_slot {
let new_last = id.index0();
self.nodes[index].data = NodeData::NextFree(Some(new_last));
Expand All @@ -407,7 +398,7 @@ impl<T> Arena<T> {
if let NodeData::NextFree(next_free) = self.nodes[index].data {
self.first_free_slot = next_free;
} else {
unreachable!("A data node considered as a freed node");
unreachable!("a data node considered as a freed node");
}
if self.first_free_slot.is_none() {
self.last_free_slot = None;
Expand All @@ -425,9 +416,6 @@ impl<T: Sync> Arena<T> {
/// Requires the `par_iter` feature. Uses [rayon](https://docs.rs/rayon)
/// for data parallelism across all nodes in storage order.
///
/// Note that this iterator returns also removed elements, which can be
/// tested with the [`is_removed()`] method on the node.
///
/// # Examples
///
/// ```
Expand All @@ -441,10 +429,8 @@ impl<T: Sync> Arena<T> {
/// let sum: i64 = arena.par_iter().map(|node| *node.get()).sum();
/// assert_eq!(sum, 6);
/// ```
///
/// [`is_removed()`]: Node::is_removed
pub fn par_iter(&self) -> rayon::slice::Iter<'_, Node<T>> {
self.nodes.par_iter()
pub fn par_iter(&self) -> impl ParallelIterator<Item = &Node<T>> {
self.nodes.par_iter().filter(|node| !node.is_removed())
}
}

Expand All @@ -458,20 +444,6 @@ impl<T> Default for Arena<T> {
}
}

impl<T> Index<NodeId> for Arena<T> {
type Output = Node<T>;

fn index(&self, node: NodeId) -> &Node<T> {
&self.nodes[node.index0()]
}
}

impl<T> IndexMut<NodeId> for Arena<T> {
fn index_mut(&mut self, node: NodeId) -> &mut Node<T> {
&mut self.nodes[node.index0()]
}
}

#[test]
fn reuse_node() {
let mut arena = Arena::new();
Expand Down
35 changes: 19 additions & 16 deletions indextree/src/debug_pretty_print.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ use alloc::vec::Vec;
use std::vec::Vec;

use crate::{
Node,
arena::Arena,
id::NodeId,
traverse::{NodeEdge, Traverse},
Expand Down Expand Up @@ -238,7 +239,7 @@ pub struct DebugPrettyPrint<'a, T> {
impl<'a, T> DebugPrettyPrint<'a, T> {
/// Creates a new `DebugPrettyPrint` object for the node.
#[inline]
pub(crate) fn new(id: &'a NodeId, arena: &'a Arena<T>) -> Self {
pub(crate) const fn new(id: &'a NodeId, arena: &'a Arena<T>) -> Self {
Self { id, arena }
}
}
Expand All @@ -251,8 +252,7 @@ impl<T: fmt::Display> fmt::Display for DebugPrettyPrint<'_, T> {

// Print the first (root) node.
traverser.next();
{
let data = self.arena[*self.id].get();
if let Some(data) = self.arena.get(*self.id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#}")?
} else {
Expand All @@ -262,11 +262,12 @@ impl<T: fmt::Display> fmt::Display for DebugPrettyPrint<'_, T> {

// Print the descendants.
while let Some(id) = prepare_next_node_printing(&mut writer, &mut traverser)? {
let data = traverser.arena()[id].get();
if is_alternate {
write!(writer, "{data:#}")?
} else {
write!(writer, "{data}")?
if let Some(data) = traverser.arena().get(id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#}")?
} else {
write!(writer, "{data}")?
}
}
}

Expand All @@ -282,8 +283,7 @@ impl<T: fmt::Debug> fmt::Debug for DebugPrettyPrint<'_, T> {

// Print the first (root) node.
traverser.next();
{
let data = self.arena[*self.id].get();
if let Some(data) = self.arena.get(*self.id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#?}")?
} else {
Expand All @@ -293,11 +293,12 @@ impl<T: fmt::Debug> fmt::Debug for DebugPrettyPrint<'_, T> {

// Print the descendants.
while let Some(id) = prepare_next_node_printing(&mut writer, &mut traverser)? {
let data = traverser.arena()[id].get();
if is_alternate {
write!(writer, "{data:#?}")?
} else {
write!(writer, "{data:?}")?
if let Some(data) = traverser.arena().get(id).map(Node::get) {
if is_alternate {
write!(writer, "{data:#?}")?
} else {
write!(writer, "{data:?}")?
}
}
}

Expand Down Expand Up @@ -325,7 +326,9 @@ fn prepare_next_node_printing<T>(
}
}
};
let is_last_sibling = traverser.arena()[id].next_sibling().is_none();
let is_last_sibling = traverser.arena().nodes[id.index0()]
Copy link
Copy Markdown
Owner

@saschagrunert saschagrunert Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id comes from a live traversal, so traverser.arena().get(id).and_then(Node::next_sibling).is_none() would work here and stay consistent with the rest of the PR.

.next_sibling()
.is_none();
writer.open_item(is_last_sibling)?;

return Ok(Some(id));
Expand Down
Loading