From a090e1f411b65236b5ca8aad3e9375426fcd075e Mon Sep 17 00:00:00 2001 From: Ulrik Sverdrup Date: Sat, 6 Jun 2015 14:26:39 +0200 Subject: [PATCH 1/5] linked_list: Use a safe loop in Drop --- src/libcollections/linked_list.rs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index 0bfbfd733771..b82a25544c93 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -626,21 +626,13 @@ impl LinkedList { #[stable(feature = "rust1", since = "1.0.0")] impl Drop for LinkedList { fn drop(&mut self) { - // Dissolve the linked_list in backwards direction + // Dissolve the linked_list in a loop. // Just dropping the list_head can lead to stack exhaustion // when length is >> 1_000_000 - let mut tail = self.list_tail; - loop { - match tail.resolve() { - None => break, - Some(prev) => { - prev.next.take(); // release Box> - tail = prev.prev; - } - } + while let Some(mut head_) = self.list_head.take() { + self.list_head = head_.next.take(); } self.length = 0; - self.list_head = None; self.list_tail = Rawlink::none(); } } From 289d5db409b40e8244c9c0ca63fc078b66da79bc Mon Sep 17 00:00:00 2001 From: Ulrik Sverdrup Date: Sat, 6 Jun 2015 14:26:39 +0200 Subject: [PATCH 2/5] linked_list: Use unsafe properly for Rawlink methods --- src/libcollections/linked_list.rs | 118 ++++++++++++++++++------------ 1 file changed, 70 insertions(+), 48 deletions(-) diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index b82a25544c93..8fbd45ea770d 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -104,17 +104,23 @@ impl Rawlink { } /// Convert the `Rawlink` into an Option value - fn resolve_immut<'a>(&self) -> Option<&'a T> { - unsafe { - self.p.as_ref() - } + /// + /// **unsafe** because: + /// + /// - Dereference of raw pointer. + /// - Returns reference of arbitrary lifetime. + unsafe fn resolve<'a>(&self) -> Option<&'a T> { + self.p.as_ref() } /// Convert the `Rawlink` into an Option value - fn resolve<'a>(&mut self) -> Option<&'a mut T> { - unsafe { - self.p.as_mut() - } + /// + /// **unsafe** because: + /// + /// - Dereference of raw pointer. + /// - Returns reference of arbitrary lifetime. + unsafe fn resolve_mut<'a>(&mut self) -> Option<&'a mut T> { + self.p.as_mut() } /// Return the `Rawlink` and replace with `Rawlink::none()` @@ -179,7 +185,7 @@ impl LinkedList { /// Add a Node last in the list #[inline] fn push_back_node(&mut self, mut new_tail: Box>) { - match self.list_tail.resolve() { + match unsafe { self.list_tail.resolve_mut() } { None => return self.push_front_node(new_tail), Some(tail) => { self.list_tail = Rawlink::some(&mut *new_tail); @@ -192,14 +198,16 @@ impl LinkedList { /// Remove the last Node and return it, or None if the list is empty #[inline] fn pop_back_node(&mut self) -> Option>> { - self.list_tail.resolve().map_or(None, |tail| { - self.length -= 1; - self.list_tail = tail.prev; - match tail.prev.resolve() { - None => self.list_head.take(), - Some(tail_prev) => tail_prev.next.take() - } - }) + unsafe { + self.list_tail.resolve_mut().and_then(|tail| { + self.length -= 1; + self.list_tail = tail.prev; + match tail.prev.resolve_mut() { + None => self.list_head.take(), + Some(tail_prev) => tail_prev.next.take() + } + }) + } } } @@ -246,7 +254,7 @@ impl LinkedList { /// ``` #[stable(feature = "rust1", since = "1.0.0")] pub fn append(&mut self, other: &mut LinkedList) { - match self.list_tail.resolve() { + match unsafe { self.list_tail.resolve_mut() } { None => { self.length = other.length; self.list_head = other.list_head.take(); @@ -433,7 +441,9 @@ impl LinkedList { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn back(&self) -> Option<&T> { - self.list_tail.resolve_immut().as_ref().map(|tail| &tail.value) + unsafe { + self.list_tail.resolve().map(|tail| &tail.value) + } } /// Provides a mutable reference to the back element, or `None` if the list @@ -460,7 +470,9 @@ impl LinkedList { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn back_mut(&mut self) -> Option<&mut T> { - self.list_tail.resolve().map(|tail| &mut tail.value) + unsafe { + self.list_tail.resolve_mut().map(|tail| &mut tail.value) + } } /// Adds an element first in the list. @@ -609,12 +621,14 @@ impl LinkedList { length: len - at }; - // Swap split_node.next with list_head (which is None), nulling out split_node.next, - // as it is the new tail. - mem::swap(&mut split_node.resolve().unwrap().next, &mut splitted_list.list_head); - // Null out list_head.prev. Note this `unwrap` won't fail because if at == len - // we already branched out at the top of the fn to return the empty list. - splitted_list.list_head.as_mut().unwrap().prev = Rawlink::none(); + unsafe { + // Swap split_node.next with list_head (which is None), nulling out split_node.next, + // as it is the new tail. + mem::swap(&mut split_node.resolve_mut().unwrap().next, &mut splitted_list.list_head); + // Null out list_head.prev. Note this `unwrap` won't fail because if at == len + // we already branched out at the top of the fn to return the empty list. + splitted_list.list_head.as_mut().unwrap().prev = Rawlink::none(); + } // Fix the tail ptr self.list_tail = split_node; self.length = at; @@ -666,11 +680,13 @@ impl<'a, A> DoubleEndedIterator for Iter<'a, A> { if self.nelem == 0 { return None; } - self.tail.resolve_immut().as_ref().map(|prev| { - self.nelem -= 1; - self.tail = prev.prev; - &prev.value - }) + unsafe { + self.tail.resolve().map(|prev| { + self.nelem -= 1; + self.tail = prev.prev; + &prev.value + }) + } } } @@ -685,14 +701,16 @@ impl<'a, A> Iterator for IterMut<'a, A> { if self.nelem == 0 { return None; } - self.head.resolve().map(|next| { - self.nelem -= 1; - self.head = match next.next { - Some(ref mut node) => Rawlink::some(&mut **node), - None => Rawlink::none(), - }; - &mut next.value - }) + unsafe { + self.head.resolve_mut().map(|next| { + self.nelem -= 1; + self.head = match next.next { + Some(ref mut node) => Rawlink::some(&mut **node), + None => Rawlink::none(), + }; + &mut next.value + }) + } } #[inline] @@ -708,11 +726,13 @@ impl<'a, A> DoubleEndedIterator for IterMut<'a, A> { if self.nelem == 0 { return None; } - self.tail.resolve().map(|prev| { - self.nelem -= 1; - self.tail = prev.prev; - &mut prev.value - }) + unsafe { + self.tail.resolve_mut().map(|prev| { + self.nelem -= 1; + self.tail = prev.prev; + &mut prev.value + }) + } } } @@ -726,10 +746,10 @@ impl<'a, A> IterMut<'a, A> { // previously yielded element and self.head. // // The inserted node will not appear in further iteration. - match self.head.resolve() { + match unsafe { self.head.resolve_mut() } { None => { self.list.push_back_node(ins_node); } Some(node) => { - let prev_node = match node.prev.resolve() { + let prev_node = match unsafe { node.prev.resolve_mut() } { None => return self.list.push_front_node(ins_node), Some(prev) => prev, }; @@ -795,7 +815,9 @@ impl<'a, A> IterMut<'a, A> { if self.nelem == 0 { return None } - self.head.resolve().map(|head| &mut head.value) + unsafe { + self.head.resolve_mut().map(|head| &mut head.value) + } } } @@ -947,7 +969,7 @@ mod tests { Some(ref node) => node_ptr = &**node, } loop { - match (last_ptr, node_ptr.prev.resolve_immut()) { + match unsafe { (last_ptr, node_ptr.prev.resolve()) } { (None , None ) => {} (None , _ ) => panic!("prev link for list_head"), (Some(p), Some(pptr)) => { From 201852e56ac1c6a2d9d050d12693df8a4b6e936f Mon Sep 17 00:00:00 2001 From: Ulrik Sverdrup Date: Sat, 6 Jun 2015 14:26:39 +0200 Subject: [PATCH 3/5] linked_list: Cleanup code in split_off --- src/libcollections/linked_list.rs | 50 +++++++++++++++++++++++-------- 1 file changed, 37 insertions(+), 13 deletions(-) diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index 8fbd45ea770d..721a3a2595e6 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -615,25 +615,29 @@ impl LinkedList { iter.tail }; - let mut splitted_list = LinkedList { - list_head: None, + // The split node is the new tail node of the first part and owns + // the head of the second part. + let mut second_part_head; + + unsafe { + second_part_head = split_node.resolve_mut().unwrap().next.take(); + match second_part_head { + None => {} + Some(ref mut head) => head.prev = Rawlink::none(), + } + } + + let second_part = LinkedList { + list_head: second_part_head, list_tail: self.list_tail, length: len - at }; - unsafe { - // Swap split_node.next with list_head (which is None), nulling out split_node.next, - // as it is the new tail. - mem::swap(&mut split_node.resolve_mut().unwrap().next, &mut splitted_list.list_head); - // Null out list_head.prev. Note this `unwrap` won't fail because if at == len - // we already branched out at the top of the fn to return the empty list. - splitted_list.list_head.as_mut().unwrap().prev = Rawlink::none(); - } - // Fix the tail ptr + // Fix the tail ptr of the first part self.list_tail = split_node; self.length = at; - splitted_list + second_part } } @@ -947,7 +951,7 @@ impl Hash for LinkedList { #[cfg(test)] mod tests { use std::clone::Clone; - use std::iter::{Iterator, IntoIterator}; + use std::iter::{Iterator, IntoIterator, Extend}; use std::option::Option::{Some, None, self}; use std::__rand::{thread_rng, Rng}; use std::thread; @@ -1115,6 +1119,26 @@ mod tests { assert_eq!(v1.iter().collect::>().len(), 3); } + #[test] + fn test_split_off() { + let mut v1 = LinkedList::new(); + v1.push_front(1u8); + v1.push_front(1u8); + v1.push_front(1u8); + v1.push_front(1u8); + + // test all splits + for ix in 0..1 + v1.len() { + let mut a = v1.clone(); + let b = a.split_off(ix); + check_links(&a); + check_links(&b); + a.extend(b); + assert_eq!(v1, a); + } + } + + #[cfg(test)] fn fuzz_test(sz: i32) { let mut m: LinkedList<_> = LinkedList::new(); From 16cefab795d37c289fe3df2e824fdf65307c6c58 Mon Sep 17 00:00:00 2001 From: Ulrik Sverdrup Date: Sat, 6 Jun 2015 14:26:40 +0200 Subject: [PATCH 4/5] linked_list: Add method Node::set_next --- src/libcollections/linked_list.rs | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index 721a3a2595e6..43cefeecdd9d 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -140,12 +140,21 @@ impl Node { fn new(v: T) -> Node { Node{value: v, next: None, prev: Rawlink::none()} } + + /// Update the `prev` link on `next`, then set self's next pointer. + /// + /// `self.next` should be `None` when you call this + /// (otherwise a Node is probably being dropped by mistake). + fn set_next(&mut self, mut next: Box>) { + debug_assert!(self.next.is_none()); + next.prev = Rawlink::some(self); + self.next = Some(next); + } } -/// Set the .prev field on `next`, then return `Some(next)` -fn link_with_prev(mut next: Box>, prev: Rawlink>) - -> Link { - next.prev = prev; +/// Clear the .prev field on `next`, then return `Some(next)` +fn link_no_prev(mut next: Box>) -> Link { + next.prev = Rawlink::none(); Some(next) } @@ -157,7 +166,7 @@ impl LinkedList { match self.list_head { None => { self.list_tail = Rawlink::some(&mut *new_head); - self.list_head = link_with_prev(new_head, Rawlink::none()); + self.list_head = link_no_prev(new_head); } Some(ref mut head) => { new_head.prev = Rawlink::none(); @@ -175,7 +184,7 @@ impl LinkedList { self.list_head.take().map(|mut front_node| { self.length -= 1; match front_node.next.take() { - Some(node) => self.list_head = link_with_prev(node, Rawlink::none()), + Some(node) => self.list_head = link_no_prev(node), None => self.list_tail = Rawlink::none() } front_node @@ -184,12 +193,12 @@ impl LinkedList { /// Add a Node last in the list #[inline] - fn push_back_node(&mut self, mut new_tail: Box>) { + fn push_back_node(&mut self, new_tail: Box>) { match unsafe { self.list_tail.resolve_mut() } { None => return self.push_front_node(new_tail), Some(tail) => { self.list_tail = Rawlink::some(&mut *new_tail); - tail.next = link_with_prev(new_tail, Rawlink::some(tail)); + tail.set_next(new_tail); } } self.length += 1; @@ -267,7 +276,7 @@ impl LinkedList { match other.list_head.take() { None => return, Some(node) => { - tail.next = link_with_prev(node, self.list_tail); + tail.set_next(node); self.list_tail = o_tail; self.length += o_length; } @@ -758,8 +767,8 @@ impl<'a, A> IterMut<'a, A> { Some(prev) => prev, }; let node_own = prev_node.next.take().unwrap(); - ins_node.next = link_with_prev(node_own, Rawlink::some(&mut *ins_node)); - prev_node.next = link_with_prev(ins_node, Rawlink::some(prev_node)); + ins_node.set_next(node_own); + prev_node.set_next(ins_node); self.list.length += 1; } } From 32037a5696272f1c34f3692dcdc59b4ada91bdc7 Mon Sep 17 00:00:00 2001 From: Ulrik Sverdrup Date: Sat, 6 Jun 2015 14:26:40 +0200 Subject: [PATCH 5/5] linked_list: Add Rawlink::from --- src/libcollections/linked_list.rs | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/src/libcollections/linked_list.rs b/src/libcollections/linked_list.rs index 43cefeecdd9d..980fe00f1e5b 100644 --- a/src/libcollections/linked_list.rs +++ b/src/libcollections/linked_list.rs @@ -129,6 +129,15 @@ impl Rawlink { } } +impl<'a, T> From<&'a mut Link> for Rawlink> { + fn from(node: &'a mut Link) -> Self { + match node.as_mut() { + None => Rawlink::none(), + Some(ptr) => Rawlink::some(ptr), + } + } +} + impl Clone for Rawlink { #[inline] fn clone(&self) -> Rawlink { @@ -165,8 +174,8 @@ impl LinkedList { fn push_front_node(&mut self, mut new_head: Box>) { match self.list_head { None => { - self.list_tail = Rawlink::some(&mut *new_head); self.list_head = link_no_prev(new_head); + self.list_tail = Rawlink::from(&mut self.list_head); } Some(ref mut head) => { new_head.prev = Rawlink::none(); @@ -197,8 +206,8 @@ impl LinkedList { match unsafe { self.list_tail.resolve_mut() } { None => return self.push_front_node(new_tail), Some(tail) => { - self.list_tail = Rawlink::some(&mut *new_tail); tail.set_next(new_tail); + self.list_tail = Rawlink::from(&mut tail.next); } } self.length += 1; @@ -297,13 +306,9 @@ impl LinkedList { #[inline] #[stable(feature = "rust1", since = "1.0.0")] pub fn iter_mut(&mut self) -> IterMut { - let head_raw = match self.list_head { - Some(ref mut h) => Rawlink::some(&mut **h), - None => Rawlink::none(), - }; - IterMut{ + IterMut { nelem: self.len(), - head: head_raw, + head: Rawlink::from(&mut self.list_head), tail: self.list_tail, list: self } @@ -717,10 +722,7 @@ impl<'a, A> Iterator for IterMut<'a, A> { unsafe { self.head.resolve_mut().map(|next| { self.nelem -= 1; - self.head = match next.next { - Some(ref mut node) => Rawlink::some(&mut **node), - None => Rawlink::none(), - }; + self.head = Rawlink::from(&mut next.next); &mut next.value }) }