From 293cdf7ac5d14811debdec3408afde104935caef Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Thu, 21 Nov 2019 22:09:10 -0500 Subject: [PATCH 1/7] Make RangeMut::next_unchecked() output a mutable key reference --- src/liballoc/collections/btree/map.rs | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 7d0a862d79e48..e25a5e0773ec6 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -1328,7 +1328,10 @@ impl<'a, K: 'a, V: 'a> Iterator for IterMut<'a, K, V> { None } else { self.length -= 1; - unsafe { Some(self.range.next_unchecked()) } + unsafe { + let (k, v) = self.range.next_unchecked(); + Some((k, v)) // coerce k from `&mut K` to `&K` + } } } @@ -1707,7 +1710,14 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> { type Item = (&'a K, &'a mut V); fn next(&mut self) -> Option<(&'a K, &'a mut V)> { - if self.front == self.back { None } else { unsafe { Some(self.next_unchecked()) } } + if self.front == self.back { + None + } else { + unsafe { + let (k, v) = self.next_unchecked(); + Some((k, v)) // coerce k from `&mut K` to `&K` + } + } } fn last(mut self) -> Option<(&'a K, &'a mut V)> { @@ -1716,7 +1726,7 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> { } impl<'a, K, V> RangeMut<'a, K, V> { - unsafe fn next_unchecked(&mut self) -> (&'a K, &'a mut V) { + unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) { let handle = ptr::read(&self.front); let mut cur_handle = match handle.right_kv() { @@ -1724,8 +1734,7 @@ impl<'a, K, V> RangeMut<'a, K, V> { self.front = ptr::read(&kv).right_edge(); // Doing the descend invalidates the references returned by `into_kv_mut`, // so we have to do this last. - let (k, v) = kv.into_kv_mut(); - return (k, v); // coerce k from `&mut K` to `&K` + return kv.into_kv_mut(); } Err(last_edge) => { let next_level = last_edge.into_node().ascend().ok(); @@ -1739,8 +1748,7 @@ impl<'a, K, V> RangeMut<'a, K, V> { self.front = first_leaf_edge(ptr::read(&kv).right_edge().descend()); // Doing the descend invalidates the references returned by `into_kv_mut`, // so we have to do this last. - let (k, v) = kv.into_kv_mut(); - return (k, v); // coerce k from `&mut K` to `&K` + return kv.into_kv_mut(); } Err(last_edge) => { let next_level = last_edge.into_node().ascend().ok(); From f547978392872684085c96a3d5c1d00bad24b724 Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Fri, 22 Nov 2019 14:22:06 -0500 Subject: [PATCH 2/7] Implement clone_from for BTree collections --- src/liballoc/collections/btree/map.rs | 54 +++++++++++++++++++++++++++ src/liballoc/collections/btree/set.rs | 13 ++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index e25a5e0773ec6..12174ffcbfa42 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -207,6 +207,60 @@ impl Clone for BTreeMap { clone_subtree(self.root.as_ref()) } } + + fn clone_from(&mut self, other: &Self) { + BTreeClone::clone_from(self, other); + } +} + +trait BTreeClone { + fn clone_from(&mut self, other: &Self); +} + +impl BTreeClone for BTreeMap { + default fn clone_from(&mut self, other: &Self) { + *self = other.clone(); + } +} + +impl BTreeClone for BTreeMap { + fn clone_from(&mut self, other: &Self) { + // This truncates `self` to `other.len()` by calling `split_off` on + // the first key after `other.len()` elements if it exists + if let Some(key) = { + if self.len() > other.len() { + let diff = self.len() - other.len(); + if diff <= other.len() { + self.iter().nth_back(diff - 1).map(|pair| (*pair.0).clone()) + } else { + self.iter().nth(other.len()).map(|pair| (*pair.0).clone()) + } + } else { + None + } + } { + self.split_off(&key); + } + let mut siter = self.range_mut(..); + let mut oiter = other.iter(); + // After truncation, `self` is at most as long as `other` so this loop + // replaces every key-value pair in `self`. Since `oiter` is in sorted + // order and the structure of the `BTreeMap` stays the same, + // the BTree invariants are maintained at the end of the loop + while siter.front != siter.back { + if let Some((ok, ov)) = oiter.next() { + // This is safe because the `siter.front != siter.back` check + // ensures that `siter` is nonempty + let (sk, sv) = unsafe { siter.next_unchecked() }; + sk.clone_from(ok); + sv.clone_from(ov); + } else { + break; + } + } + // If `other` is longer than `self`, the remaining elements are inserted + self.extend(oiter.map(|(k, v)| ((*k).clone(), (*v).clone()))); + } } impl super::Recover for BTreeMap diff --git a/src/liballoc/collections/btree/set.rs b/src/liballoc/collections/btree/set.rs index 282d163141bc8..5bdefe5cecf1b 100644 --- a/src/liballoc/collections/btree/set.rs +++ b/src/liballoc/collections/btree/set.rs @@ -56,12 +56,23 @@ use crate::collections::btree_map::{self, BTreeMap, Keys}; /// println!("{}", book); /// } /// ``` -#[derive(Clone, Hash, PartialEq, Eq, Ord, PartialOrd)] +#[derive(Hash, PartialEq, Eq, Ord, PartialOrd)] #[stable(feature = "rust1", since = "1.0.0")] pub struct BTreeSet { map: BTreeMap, } +#[stable(feature = "rust1", since = "1.0.0")] +impl Clone for BTreeSet { + fn clone(&self) -> Self { + BTreeSet { map: self.map.clone() } + } + + fn clone_from(&mut self, other: &Self) { + self.map.clone_from(&other.map); + } +} + /// An iterator over the items of a `BTreeSet`. /// /// This `struct` is created by the [`iter`] method on [`BTreeSet`]. From 8651aa066fdbbcfaa082531969469c3fa289de9e Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Fri, 22 Nov 2019 14:22:45 -0500 Subject: [PATCH 3/7] Add test for BTreeMap::clone_from --- src/liballoc/tests/btree/map.rs | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/liballoc/tests/btree/map.rs b/src/liballoc/tests/btree/map.rs index 3177f19927eda..65a3fbd66a4c2 100644 --- a/src/liballoc/tests/btree/map.rs +++ b/src/liballoc/tests/btree/map.rs @@ -554,6 +554,26 @@ fn test_clone() { } } +#[test] +fn test_clone_from() { + let mut map1 = BTreeMap::new(); + let size = 30; + + for i in 0..size { + map1.insert(i, 10 * i); + let mut map2 = BTreeMap::new(); + for j in 0..i { + map2.insert(100 * j + 1, 2 * j + 1); + let mut map1_copy = map2.clone(); + map1_copy.clone_from(&map1); + assert_eq!(map1_copy, map1); + let mut map2_copy = map1.clone(); + map2_copy.clone_from(&map2); + assert_eq!(map2_copy, map2); + } + } +} + #[test] #[allow(dead_code)] fn test_variance() { From 3caa17b468e50c885f56e7872200e9a76815462b Mon Sep 17 00:00:00 2001 From: Charles Gleason <36055314+crgl@users.noreply.github.com> Date: Tue, 28 Jan 2020 11:27:31 -0500 Subject: [PATCH 4/7] Format safety comment as per tidy Co-Authored-By: Ashley Mannix --- src/liballoc/collections/btree/map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 12174ffcbfa42..58cb561938acc 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -249,7 +249,7 @@ impl BTreeClone for BTreeMap { // the BTree invariants are maintained at the end of the loop while siter.front != siter.back { if let Some((ok, ov)) = oiter.next() { - // This is safe because the `siter.front != siter.back` check + // SAFETY: This is safe because the `siter.front != siter.back` check // ensures that `siter` is nonempty let (sk, sv) = unsafe { siter.next_unchecked() }; sk.clone_from(ok); From 60a7c9421e78a29610e019b4030ca011bcd0bfd5 Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Tue, 28 Jan 2020 11:39:48 -0500 Subject: [PATCH 5/7] Include empty BTreeMap in clone_from tests --- src/liballoc/tests/btree/map.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/liballoc/tests/btree/map.rs b/src/liballoc/tests/btree/map.rs index 65a3fbd66a4c2..cd9daf41eb1ed 100644 --- a/src/liballoc/tests/btree/map.rs +++ b/src/liballoc/tests/btree/map.rs @@ -560,17 +560,17 @@ fn test_clone_from() { let size = 30; for i in 0..size { - map1.insert(i, 10 * i); let mut map2 = BTreeMap::new(); for j in 0..i { - map2.insert(100 * j + 1, 2 * j + 1); let mut map1_copy = map2.clone(); map1_copy.clone_from(&map1); assert_eq!(map1_copy, map1); let mut map2_copy = map1.clone(); map2_copy.clone_from(&map2); assert_eq!(map2_copy, map2); + map2.insert(100 * j + 1, 2 * j + 1); } + map1.insert(i, 10 * i); } } From 81b6f8c3fc9098e7b1b6aad230ea7770d03070bc Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Tue, 28 Jan 2020 11:46:49 -0500 Subject: [PATCH 6/7] Add private is_empty method to RangeMut --- src/liballoc/collections/btree/map.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index 58cb561938acc..d0c56d83d722a 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -247,7 +247,7 @@ impl BTreeClone for BTreeMap { // replaces every key-value pair in `self`. Since `oiter` is in sorted // order and the structure of the `BTreeMap` stays the same, // the BTree invariants are maintained at the end of the loop - while siter.front != siter.back { + while !siter.is_empty() { if let Some((ok, ov)) = oiter.next() { // SAFETY: This is safe because the `siter.front != siter.back` check // ensures that `siter` is nonempty @@ -1764,7 +1764,7 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> { type Item = (&'a K, &'a mut V); fn next(&mut self) -> Option<(&'a K, &'a mut V)> { - if self.front == self.back { + if self.is_empty() { None } else { unsafe { @@ -1780,6 +1780,10 @@ impl<'a, K, V> Iterator for RangeMut<'a, K, V> { } impl<'a, K, V> RangeMut<'a, K, V> { + fn is_empty(&self) -> bool { + self.front == self.back + } + unsafe fn next_unchecked(&mut self) -> (&'a mut K, &'a mut V) { let handle = ptr::read(&self.front); @@ -1816,7 +1820,7 @@ impl<'a, K, V> RangeMut<'a, K, V> { #[stable(feature = "btree_range", since = "1.17.0")] impl<'a, K, V> DoubleEndedIterator for RangeMut<'a, K, V> { fn next_back(&mut self) -> Option<(&'a K, &'a mut V)> { - if self.front == self.back { None } else { unsafe { Some(self.next_back_unchecked()) } } + if self.is_empty() { None } else { unsafe { Some(self.next_back_unchecked()) } } } } From 6c3e477d134511094ab301fc15c504cc79804e41 Mon Sep 17 00:00:00 2001 From: Charles Gleason Date: Tue, 28 Jan 2020 11:59:50 -0500 Subject: [PATCH 7/7] Reformat truncation section of clone_from --- src/liballoc/collections/btree/map.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/liballoc/collections/btree/map.rs b/src/liballoc/collections/btree/map.rs index d0c56d83d722a..7c45a6fe49823 100644 --- a/src/liballoc/collections/btree/map.rs +++ b/src/liballoc/collections/btree/map.rs @@ -227,20 +227,20 @@ impl BTreeClone for BTreeMap { fn clone_from(&mut self, other: &Self) { // This truncates `self` to `other.len()` by calling `split_off` on // the first key after `other.len()` elements if it exists - if let Some(key) = { - if self.len() > other.len() { - let diff = self.len() - other.len(); - if diff <= other.len() { - self.iter().nth_back(diff - 1).map(|pair| (*pair.0).clone()) - } else { - self.iter().nth(other.len()).map(|pair| (*pair.0).clone()) - } + let split_off_key = if self.len() > other.len() { + let diff = self.len() - other.len(); + if diff <= other.len() { + self.iter().nth_back(diff - 1).map(|pair| (*pair.0).clone()) } else { - None + self.iter().nth(other.len()).map(|pair| (*pair.0).clone()) } - } { + } else { + None + }; + if let Some(key) = split_off_key { self.split_off(&key); } + let mut siter = self.range_mut(..); let mut oiter = other.iter(); // After truncation, `self` is at most as long as `other` so this loop