Skip to content

BTreeMap: revert a part of #78015 and test what it broke #78397

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.

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
30 changes: 23 additions & 7 deletions library/alloc/src/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,13 @@ use core::borrow::Borrow;
use core::cmp::Ordering;
use core::fmt::{self, Debug};
use core::hash::{Hash, Hasher};
use core::iter::{FromIterator, FusedIterator};
use core::iter::{FromIterator, FusedIterator, Peekable};
use core::marker::PhantomData;
use core::mem::{self, ManuallyDrop};
use core::ops::{Index, RangeBounds};
use core::ptr;

use super::borrow::DormantMutRef;
use super::merge_iter::MergeIterInner;
use super::node::{self, marker, ForceResult::*, Handle, NodeRef};
use super::search::{self, SearchResult::*};
use super::unwrap_unchecked;
Expand Down Expand Up @@ -459,7 +458,10 @@ impl<K: fmt::Debug, V: fmt::Debug> fmt::Debug for RangeMut<'_, K, V> {
}

// An iterator for merging two sorted sequences into one
struct MergeIter<K, V, I: Iterator<Item = (K, V)>>(MergeIterInner<I>);
struct MergeIter<K, V, I: Iterator<Item = (K, V)>> {
left: Peekable<I>,
right: Peekable<I>,
}

impl<K: Ord, V> BTreeMap<K, V> {
/// Makes a new empty BTreeMap.
Expand Down Expand Up @@ -911,7 +913,7 @@ impl<K: Ord, V> BTreeMap<K, V> {
// First, we merge `self` and `other` into a sorted sequence in linear time.
let self_iter = mem::take(self).into_iter();
let other_iter = mem::take(other).into_iter();
let iter = MergeIter(MergeIterInner::new(self_iter, other_iter));
let iter = MergeIter { left: self_iter.peekable(), right: other_iter.peekable() };

// Second, we build a tree from the sorted sequence in linear time.
self.from_sorted_iter(iter);
Expand Down Expand Up @@ -2224,10 +2226,24 @@ where
{
type Item = (K, V);

/// If two keys are equal, returns the key/value-pair from the right source.
fn next(&mut self) -> Option<(K, V)> {
let (a_next, b_next) = self.0.nexts(|a: &(K, V), b: &(K, V)| K::cmp(&a.0, &b.0));
b_next.or(a_next)
let res = match (self.left.peek(), self.right.peek()) {
(Some(&(ref left_key, _)), Some(&(ref right_key, _))) => left_key.cmp(right_key),
(Some(_), None) => Ordering::Less,
(None, Some(_)) => Ordering::Greater,
(None, None) => return None,
};

// Check which elements comes first and only advance the corresponding iterator.
// If two keys are equal, take the value from `right`.
match res {
Ordering::Less => self.left.next(),
Ordering::Greater => self.right.next(),
Ordering::Equal => {
self.left.next();
self.right.next()
}
}
}
}

Expand Down
27 changes: 27 additions & 0 deletions library/alloc/src/collections/btree/map/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,33 @@ create_append_test!(test_append_239, 239);
#[cfg(not(miri))] // Miri is too slow
create_append_test!(test_append_1700, 1700);

#[test]
fn test_append_drop_leak() {
static DROPS: AtomicUsize = AtomicUsize::new(0);

struct D;

impl Drop for D {
fn drop(&mut self) {
if DROPS.fetch_add(1, Ordering::SeqCst) == 0 {
panic!("panic in `drop`");
}
}
}

let mut left = BTreeMap::new();
let mut right = BTreeMap::new();
left.insert(0, D);
left.insert(1, D); // first to be dropped during append
left.insert(2, D);
right.insert(1, D);
right.insert(2, D);

catch_unwind(move || left.append(&mut right)).unwrap_err();

assert_eq!(DROPS.load(Ordering::SeqCst), 5);
}

fn rand_data(len: usize) -> Vec<(u32, u32)> {
let mut rng = DeterministicRng::new();
Vec::from_iter((0..len).map(|_| (rng.next(), rng.next())))
Expand Down