Skip to content

Commit c4807de

Browse files
committed
give Node an unsafe constructor
`Node` is only safe if it doesn't repeat any indexes into the `ItemSliceSync`. Let's document this by giving it an unsafe constructor. To enforce that, I also moved it into a private module.
1 parent 43bca11 commit c4807de

File tree

1 file changed

+49
-34
lines changed

1 file changed

+49
-34
lines changed

gix-pack/src/cache/delta/traverse/resolve.rs

Lines changed: 49 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -14,45 +14,57 @@ use crate::{
1414
data::EntryRange,
1515
};
1616

17-
/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
18-
struct Node<'a, T: Send> {
19-
item: &'a mut Item<T>,
20-
child_items: &'a ItemSliceSync<'a, Item<T>>,
21-
}
17+
mod node {
18+
use crate::cache::delta::{traverse::util::ItemSliceSync, Item};
2219

23-
impl<'a, T: Send> Node<'a, T> {
24-
/// Returns the offset into the pack at which the `Node`s data is located.
25-
pub fn offset(&self) -> u64 {
26-
self.item.offset
20+
/// An item returned by `iter_root_chunks`, allowing access to the `data` stored alongside nodes in a [`Tree`].
21+
pub(crate) struct Node<'a, T: Send> {
22+
item: &'a mut Item<T>,
23+
child_items: &'a ItemSliceSync<'a, Item<T>>,
2724
}
2825

29-
/// Returns the slice into the data pack at which the pack entry is located.
30-
pub fn entry_slice(&self) -> crate::data::EntryRange {
31-
self.item.offset..self.item.next_offset
26+
impl<'a, T: Send> Node<'a, T> {
27+
/// SAFETY: The child_items must be unique among between users of the `ItemSliceSync`.
28+
#[allow(unsafe_code)]
29+
pub(crate) unsafe fn new(item: &'a mut Item<T>, child_items: &'a ItemSliceSync<'a, Item<T>>) -> Self {
30+
Node { item, child_items }
31+
}
3232
}
3333

34-
/// Returns the node data associated with this node.
35-
pub fn data(&mut self) -> &mut T {
36-
&mut self.item.data
37-
}
34+
impl<'a, T: Send> Node<'a, T> {
35+
/// Returns the offset into the pack at which the `Node`s data is located.
36+
pub fn offset(&self) -> u64 {
37+
self.item.offset
38+
}
3839

39-
/// Returns true if this node has children, e.g. is not a leaf in the tree.
40-
pub fn has_children(&self) -> bool {
41-
!self.item.children.is_empty()
42-
}
40+
/// Returns the slice into the data pack at which the pack entry is located.
41+
pub fn entry_slice(&self) -> crate::data::EntryRange {
42+
self.item.offset..self.item.next_offset
43+
}
4344

44-
/// Transform this `Node` into an iterator over its children.
45-
///
46-
/// Children are `Node`s referring to pack entries whose base object is this pack entry.
47-
pub fn into_child_iter(self) -> impl Iterator<Item = Node<'a, T>> + 'a {
48-
let children = self.child_items;
49-
// SAFETY: The index is a valid index into the children array.
50-
// SAFETY: The resulting mutable pointer cannot be yielded by any other node.
51-
#[allow(unsafe_code)]
52-
self.item.children.iter().map(move |&index| Node {
53-
item: unsafe { children.get_mut(index as usize) },
54-
child_items: children,
55-
})
45+
/// Returns the node data associated with this node.
46+
pub fn data(&mut self) -> &mut T {
47+
&mut self.item.data
48+
}
49+
50+
/// Returns true if this node has children, e.g. is not a leaf in the tree.
51+
pub fn has_children(&self) -> bool {
52+
!self.item.children.is_empty()
53+
}
54+
55+
/// Transform this `Node` into an iterator over its children.
56+
///
57+
/// Children are `Node`s referring to pack entries whose base object is this pack entry.
58+
pub fn into_child_iter(self) -> impl Iterator<Item = Node<'a, T>> + 'a {
59+
let children = self.child_items;
60+
// SAFETY: The index is a valid index into the children array.
61+
// SAFETY: The resulting mutable pointer cannot be yielded by any other node.
62+
#[allow(unsafe_code)]
63+
self.item.children.iter().map(move |&index| Node {
64+
item: unsafe { children.get_mut(index as usize) },
65+
child_items: children,
66+
})
67+
}
5668
}
5769
}
5870

@@ -106,7 +118,10 @@ where
106118
// each node is a base, and its children always start out as deltas which become a base after applying them.
107119
// These will be pushed onto our stack until all are processed
108120
let root_level = 0;
109-
let mut nodes: Vec<_> = vec![(root_level, Node { item, child_items })];
121+
// SAFETY: The child items are unique
122+
#[allow(unsafe_code)]
123+
let root_node = unsafe { node::Node::new(item, child_items) };
124+
let mut nodes: Vec<_> = vec![(root_level, root_node)];
110125
while let Some((level, mut base)) = nodes.pop() {
111126
if should_interrupt.load(Ordering::Relaxed) {
112127
return Err(Error::Interrupted);
@@ -225,7 +240,7 @@ fn deltas_mt<T, F, MBFN, E, R>(
225240
objects: gix_features::progress::StepShared,
226241
size: gix_features::progress::StepShared,
227242
progress: &dyn Progress,
228-
nodes: Vec<(u16, Node<'_, T>)>,
243+
nodes: Vec<(u16, node::Node<'_, T>)>,
229244
resolve: F,
230245
resolve_data: &R,
231246
modify_base: MBFN,

0 commit comments

Comments
 (0)