Auto merge of #79376 - ssomers:btree_choose_parent_kv, r=Mark-Simulacrum

BTreeMap: clarify comments and panics around choose_parent_kv

Fixes a lie in recent code: `unreachable!("empty non-root node")` should shout "empty internal node", but it might as well be good and keep quiet

r? `@Mark-Simulacrum`

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 ae5831d5140..07667fa2288 100644
--- a/library/alloc/src/collections/btree/node.rs
+++ b/library/alloc/src/collections/btree/node.rs
@@ -1288,10 +1288,12 @@ impl<'a, K, V> NodeRef<marker::Mut<'a>, 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
@@ -1299,19 +1301,19 @@ impl<'a, K, V> NodeRef<marker::Mut<'a>, K, V, marker::LeafOrInternal> {
     /// the left.
     pub fn choose_parent_kv(self) -> Result<LeftOrRight<BalancingContext<'a, K, V>>, 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 1da0a060806..7aeb39cbc32 100644
--- a/library/alloc/src/collections/btree/remove.rs
+++ b/library/alloc/src/collections/btree/remove.rs
@@ -54,12 +54,12 @@ impl<'a, K: 'a, V: 'a> Handle<NodeRef<marker::Mut<'a>, 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);
             }
@@ -90,8 +90,8 @@ impl<'a, K: 'a, V: 'a> Handle<NodeRef<marker::Mut<'a>, K, V, marker::Internal>,
 }
 
 impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, 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<F: FnOnce()>(mut self, handle_emptied_internal_root: F) {
         loop {
             self = match self.len() {
@@ -122,7 +122,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, K, V, marker::Internal> {
     ) -> Option<NodeRef<marker::Mut<'a>, 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()) };
@@ -134,7 +134,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, 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()) };