[Features] Iterator over mutable childs, deep cloning for nodes #46
Conversation
src/iter.rs
Outdated
| impl<'a, T: 'a> Iterator for ChildrenMut<'a, T> { | ||
| type Item = NodeMut<'a, T>; | ||
| fn next(&mut self) -> Option<Self::Item> { | ||
| // lifetime cast here is safe because &mut self living shorter than 'a |
There was a problem hiding this comment.
Please mark this as // SAFETY: comment.
There is also the aliasing question to discuss, i.e. the mutable reference must not only be valid but also exclusive.
There was a problem hiding this comment.
You are right, there is no exclusivity here. Do you have any recommendations on how to fix this?
Now I realize there was no children_mut here for a good reason :{
There was a problem hiding this comment.
So I guess the first problem I see is that this will yield two children who can then above mutably access their parent via NodeMut::parent?
I don't think you can fix this using an Iterator of NodeMut. You would either need to use a different trait, e.g. a lending iterator which prevents have both NodeMut alive at the same time or return a different item which cannot navigate in the problematic directions.
I guess both changes would appear to limit the appeal of the API somewhat though?
src/lib.rs
Outdated
| let copy_id = id_map.get(&orig.id).unwrap(); | ||
| let mut copy = unsafe { result.get_unchecked_mut(*copy_id) }; | ||
| for child in orig.children() { | ||
| id_map.insert(child.id, copy.append(child.value().clone()).id); |
There was a problem hiding this comment.
Please put copying, appending and map insertion into separate lines to make this easier to read.
src/lib.rs
Outdated
| pub fn deep_clone(&self, id: NodeId) -> Self { | ||
| let start_node = unsafe { self.get_unchecked(id) }; | ||
| let mut result = Self::new(start_node.value().clone()); | ||
| let mut id_map = std::collections::HashMap::new(); |
There was a problem hiding this comment.
I am not sure I understand why this is necessary? Wouldn't a breadth-first copy using a simple stack suffice here?
There was a problem hiding this comment.
I have an argument for this: when iterating over children, it is better if they are locate one after another. This prevents cache misses and works a bit faster.
In my case it doesn't matter. I can change it if you say so.
There was a problem hiding this comment.
when iterating over children, it is better if they are locate one after another.
I think a breadth-first (but not depth-first) traversal would also produce that property. So "stack" was wrong in my comment, you want a queue, e.g. VecDeque.
This PR contains following changes: