Skip to content
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

BTreeMap mutable iterators should not take any reference to visited nodes during iteration #73971

Merged
merged 2 commits into from
Sep 9, 2020
Merged
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
4 changes: 2 additions & 2 deletions library/alloc/src/collections/btree/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2796,8 +2796,8 @@ enum UnderflowResult<'a, K, V> {
Stole(bool),
}

fn handle_underfull_node<K, V>(
node: NodeRef<marker::Mut<'_>, K, V, marker::LeafOrInternal>,
fn handle_underfull_node<'a, K: 'a, V: 'a>(
node: NodeRef<marker::Mut<'a>, K, V, marker::LeafOrInternal>,
) -> UnderflowResult<'_, K, V> {
let parent = match node.ascend() {
Ok(parent) => parent,
Expand Down
15 changes: 9 additions & 6 deletions library/alloc/src/collections/btree/map/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ impl<'a, K: 'a, V: 'a> BTreeMap<K, V> {
let min_len = if is_root { 0 } else { node::MIN_LEN };
assert!(node.len() >= min_len, "{} < {}", node.len(), min_len);

for &key in node.keys() {
for idx in 0..node.len() {
let key = *unsafe { node.key_at(idx) };
checker.is_ascending(key);
}
leaf_length += node.len();
Expand Down Expand Up @@ -120,7 +121,13 @@ impl<'a, K: 'a, V: 'a> BTreeMap<K, V> {
Position::Leaf(leaf) => {
let depth = root_node.height();
let indent = " ".repeat(depth);
result += &format!("\n{}{:?}", indent, leaf.keys())
result += &format!("\n{}", indent);
for idx in 0..leaf.len() {
if idx > 0 {
result += ", ";
}
result += &format!("{:?}", unsafe { leaf.key_at(idx) });
}
}
Position::Internal(_) => {}
Position::InternalKV(kv) => {
Expand Down Expand Up @@ -432,7 +439,6 @@ fn test_iter_mut_mutation() {
}

#[test]
#[cfg_attr(miri, ignore)] // FIXME: fails in Miri <https://github.com/rust-lang/rust/issues/73915>
fn test_values_mut() {
let mut a: BTreeMap<_, _> = (0..MIN_INSERTS_HEIGHT_2).map(|i| (i, i)).collect();
test_all_refs(&mut 13, a.values_mut());
Expand All @@ -455,7 +461,6 @@ fn test_values_mut_mutation() {
}

#[test]
#[cfg_attr(miri, ignore)] // FIXME: fails in Miri <https://github.com/rust-lang/rust/issues/73915>
fn test_iter_entering_root_twice() {
let mut map: BTreeMap<_, _> = (0..2).map(|i| (i, i)).collect();
let mut it = map.iter_mut();
Expand All @@ -471,7 +476,6 @@ fn test_iter_entering_root_twice() {
}

#[test]
#[cfg_attr(miri, ignore)] // FIXME: fails in Miri <https://github.com/rust-lang/rust/issues/73915>
fn test_iter_descending_to_same_node_twice() {
let mut map: BTreeMap<_, _> = (0..MIN_INSERTS_HEIGHT_1).map(|i| (i, i)).collect();
let mut it = map.iter_mut();
Expand Down Expand Up @@ -515,7 +519,6 @@ fn test_iter_mixed() {
}

#[test]
#[cfg_attr(miri, ignore)] // FIXME: fails in Miri <https://github.com/rust-lang/rust/issues/73915>
fn test_iter_min_max() {
let mut a = BTreeMap::new();
assert_eq!(a.iter().min(), None);
Expand Down
8 changes: 2 additions & 6 deletions library/alloc/src/collections/btree/navigate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,6 @@ impl<'a, K, V> Handle<NodeRef<marker::Immut<'a>, K, V, marker::Leaf>, marker::Ed
impl<'a, K, V> Handle<NodeRef<marker::ValMut<'a>, K, V, marker::Leaf>, marker::Edge> {
/// Moves the leaf edge handle to the next leaf edge and returns references to the
/// key and value in between.
/// The returned references might be invalidated when the updated handle is used again.
///
/// # Safety
/// There must be another KV in the direction travelled.
Expand All @@ -376,14 +375,12 @@ impl<'a, K, V> Handle<NodeRef<marker::ValMut<'a>, K, V, marker::Leaf>, marker::E
let kv = unsafe { unwrap_unchecked(kv.ok()) };
(unsafe { ptr::read(&kv) }.next_leaf_edge(), kv)
});
// Doing the descend (and perhaps another move) invalidates the references
// returned by `into_kv_valmut`, so we have to do this last.
// Doing this last is faster, according to benchmarks.
kv.into_kv_valmut()
}

/// Moves the leaf edge handle to the previous leaf and returns references to the
/// key and value in between.
/// The returned references might be invalidated when the updated handle is used again.
///
/// # Safety
/// There must be another KV in the direction travelled.
Expand All @@ -393,8 +390,7 @@ impl<'a, K, V> Handle<NodeRef<marker::ValMut<'a>, K, V, marker::Leaf>, marker::E
let kv = unsafe { unwrap_unchecked(kv.ok()) };
(unsafe { ptr::read(&kv) }.next_back_leaf_edge(), kv)
});
// Doing the descend (and perhaps another move) invalidates the references
// returned by `into_kv_valmut`, so we have to do this last.
// Doing this last is faster, according to benchmarks.
kv.into_kv_valmut()
}
}
Expand Down
Loading