From 50576420f559be3c681e6833327845d6cf3217b4 Mon Sep 17 00:00:00 2001 From: Stein Somers Date: Tue, 24 Nov 2020 12:27:38 +0100 Subject: [PATCH] BTreeMap: clarify comments and panics surrounding choose_parent_kv --- library/alloc/src/collections/btree/node.rs | 14 ++++++++------ library/alloc/src/collections/btree/remove.rs | 12 ++++++------ 2 files changed, 14 insertions(+), 12 deletions(-) diff --git a/library/alloc/src/collections/btree/node.rs b/library/alloc/src/collections/btree/node.rs index 31809fde57b..588066f9fec 100644 --- a/library/alloc/src/collections/btree/node.rs +++ b/library/alloc/src/collections/btree/node.rs @@ -1282,10 +1282,12 @@ impl<'a, K, V> NodeRef, K, V, marker::LeafOrInternal> { /// Chooses a balancing context involving the node as a child, thus between /// the KV immediately to the left or to the right in the parent node. /// Returns an `Err` if there is no parent. + /// Panics if the parent is empty. /// - /// This method optimizes for a node that has fewer elements than its left - /// and right siblings, if they exist, by preferring the left parent KV. - /// Merging with the left sibling is faster, since we only need to move + /// Prefers the left side, to be optimal if the given node is somehow + /// underfull, meaning here only that it has fewer elements than its left + /// sibling and than its right sibling, if they exist. In that case, + /// merging with the left sibling is faster, since we only need to move /// the node's N elements, instead of shifting them to the right and moving /// more than N elements in front. Stealing from the left sibling is also /// typically faster, since we only need to shift the node's N elements to @@ -1293,19 +1295,19 @@ impl<'a, K, V> NodeRef, K, V, marker::LeafOrInternal> { /// the left. pub fn choose_parent_kv(self) -> Result>, Self> { match unsafe { ptr::read(&self) }.ascend() { - Ok(parent) => match parent.left_kv() { + Ok(parent_edge) => match parent_edge.left_kv() { Ok(left_parent_kv) => Ok(LeftOrRight::Left(BalancingContext { parent: unsafe { ptr::read(&left_parent_kv) }, left_child: left_parent_kv.left_edge().descend(), right_child: self, })), - Err(parent) => match parent.right_kv() { + Err(parent_edge) => match parent_edge.right_kv() { Ok(right_parent_kv) => Ok(LeftOrRight::Right(BalancingContext { parent: unsafe { ptr::read(&right_parent_kv) }, left_child: self, right_child: right_parent_kv.right_edge().descend(), })), - Err(_) => unreachable!("empty non-root node"), + Err(_) => unreachable!("empty internal node"), }, }, Err(root) => Err(root), diff --git a/library/alloc/src/collections/btree/remove.rs b/library/alloc/src/collections/btree/remove.rs index 5ae1c1fcab8..d8a0d51f28a 100644 --- a/library/alloc/src/collections/btree/remove.rs +++ b/library/alloc/src/collections/btree/remove.rs @@ -55,12 +55,12 @@ impl<'a, K: 'a, V: 'a> Handle, K, V, marker::Leaf>, mark pos = unsafe { new_pos.cast_to_leaf_unchecked() }; // Only if we merged, the parent (if any) has shrunk, but skipping - // the following step does not pay off in benchmarks. + // the following step otherwise does not pay off in benchmarks. // // SAFETY: We won't destroy or rearrange the leaf where `pos` is at // by handling its parent recursively; at worst we will destroy or // rearrange the parent through the grandparent, thus change the - // leaf's parent pointer. + // link to the parent inside the leaf. if let Ok(parent) = unsafe { pos.reborrow_mut() }.into_node().ascend() { parent.into_node().handle_shrunk_node_recursively(handle_emptied_internal_root); } @@ -92,8 +92,8 @@ impl<'a, K: 'a, V: 'a> Handle, K, V, marker::Internal>, } impl<'a, K: 'a, V: 'a> NodeRef, K, V, marker::Internal> { - /// Stocks up a possibly underfull internal node, recursively. - /// Climbs up until it reaches an ancestor that has elements to spare or the root. + /// Stocks up a possibly underfull internal node and its ancestors, + /// until it reaches an ancestor that has elements to spare or is the root. fn handle_shrunk_node_recursively(mut self, handle_emptied_internal_root: F) { loop { self = match self.len() { @@ -124,7 +124,7 @@ impl<'a, K: 'a, V: 'a> NodeRef, K, V, marker::Internal> { ) -> Option, K, V, marker::Internal>> { match self.forget_type().choose_parent_kv() { Ok(Left(left_parent_kv)) => { - debug_assert!(left_parent_kv.right_child_len() == MIN_LEN - 1); + debug_assert_eq!(left_parent_kv.right_child_len(), MIN_LEN - 1); if left_parent_kv.can_merge() { let pos = left_parent_kv.merge(None); let parent_edge = unsafe { unwrap_unchecked(pos.into_node().ascend().ok()) }; @@ -136,7 +136,7 @@ impl<'a, K: 'a, V: 'a> NodeRef, K, V, marker::Internal> { } } Ok(Right(right_parent_kv)) => { - debug_assert!(right_parent_kv.left_child_len() == MIN_LEN - 1); + debug_assert_eq!(right_parent_kv.left_child_len(), MIN_LEN - 1); if right_parent_kv.can_merge() { let pos = right_parent_kv.merge(None); let parent_edge = unsafe { unwrap_unchecked(pos.into_node().ascend().ok()) };