add comments and make tests easier to read

antonilol · antonilol · commit a3ebd87330ce · 2025-10-23T13:49:23.000+02:00
diff --git a/library/alloc/src/collections/vec_deque/mod.rs b/library/alloc/src/collections/vec_deque/mod.rs
@@ -266,6 +266,8 @@ impl<T, A: Allocator> VecDeque<T, A> {
         let src_wraps = room_after_src < count;
         let dst_wraps = room_after_dst < count;
 
+        // Wrapping occurs if `capacity` is contained within `wrapped_src..wrapped_src + count` or `wrapped_dst..wrapped_dst + count`.
+        // Since these two ranges must not overlap as per the safety invariants of this function, only one range can wrap.
         debug_assert!(
             !(src_wraps && dst_wraps),
             "BUG: at most one of src and dst can wrap. src={src} dst={dst} count={count} cap={}",
@@ -3150,6 +3152,10 @@ impl<T: Clone, A: Allocator> SpecExtendFromWithin for VecDeque<T, A> {
             // - Ranges do not overlap: src entirely spans initialized values, dst entirely spans uninitialized values.
             // - Ranges are in bounds: guaranteed by the caller.
             let ranges = self.nonoverlapping_ranges(src, dst, count, self.head);
+
+            // `len` is updated after every clone to prevent leaking and
+            // leave the deque in the right state when a clone implementation panics
+
             for (src, dst, count) in ranges {
                 for offset in 0..count {
                     dst.add(offset).write((*src.add(offset)).clone());
@@ -3172,23 +3178,33 @@ impl<T: Clone, A: Allocator> SpecExtendFromWithin for VecDeque<T, A> {
             // - Ranges do not overlap: src entirely spans initialized values, dst entirely spans uninitialized values.
             // - Ranges are in bounds: guaranteed by the caller.
             let ranges = self.nonoverlapping_ranges(src, dst, count, new_head);
+
+            // Cloning is done in reverse because we prepend to the front of the deque,
+            // we can't get holes in the *logical* buffer.
+            // `head` and `len` are updated after every clone to prevent leaking and
+            // leave the deque in the right state when a clone implementation panics
+
+            // Clone the first range
             let (src, dst, count) = ranges[1];
             for offset in (0..count).rev() {
                 dst.add(offset).write((*src.add(offset)).clone());
                 self.head -= 1;
                 self.len += 1;
             }
 
+            // Clone the second range
             let (src, dst, count) = ranges[0];
             let mut iter = (0..count).rev();
             if let Some(offset) = iter.next() {
                 dst.add(offset).write((*src.add(offset)).clone());
+                // After the first clone of the second range, wrap `head` around
                 if self.head == 0 {
                     self.head = cap;
                 }
                 self.head -= 1;
                 self.len += 1;
 
+                // Continue like normal
                 for offset in iter {
                     dst.add(offset).write((*src.add(offset)).clone());
                     self.head -= 1;
diff --git a/library/alloctests/tests/vec_deque.rs b/library/alloctests/tests/vec_deque.rs
@@ -1856,77 +1856,99 @@ fn test_extend_from_within() {
     let mut v = VecDeque::with_capacity(8);
     v.extend(0..6);
     v.truncate_front(4);
+    assert_eq!(v, [2, 3, 4, 5]);
     v.extend_from_within(1..4);
+    assert_eq!(v, [2, 3, 4, 5, 3, 4, 5]);
+    // check it really wrapped
     assert_eq!(v.as_slices(), ([2, 3, 4, 5, 3, 4].as_slice(), [5].as_slice()));
     v.extend_from_within(1..=2);
     assert_eq!(v, [2, 3, 4, 5, 3, 4, 5, 3, 4]);
     v.extend_from_within(..3);
     assert_eq!(v, [2, 3, 4, 5, 3, 4, 5, 3, 4, 2, 3, 4]);
 }
 
+/// Struct that allows tracking clone and drop calls and can be set to panic on calling clone.
 struct CloneTracker<'a> {
-    clone: &'a Cell<u32>,
-    drop: &'a Cell<u32>,
+    id: usize,
+    // Counters can be set to None if not needed.
+    clone: Option<&'a Cell<u32>>,
+    drop: Option<&'a Cell<u32>>,
     panic: bool,
 }
 
+impl<'a> CloneTracker<'a> {
+    pub const DUMMY: Self = Self { id: 999, clone: None, drop: None, panic: false };
+}
+
 impl<'a> Clone for CloneTracker<'a> {
     fn clone(&self) -> Self {
-        self.clone.set(self.clone.get() + 1);
         if self.panic {
             panic!();
         }
-        Self { clone: self.clone, drop: self.drop, panic: false }
+
+        if let Some(clone_count) = self.clone {
+            clone_count.update(|c| c + 1);
+        }
+
+        Self { id: self.id, clone: self.clone, drop: self.drop, panic: false }
     }
 }
 
 impl<'a> Drop for CloneTracker<'a> {
     fn drop(&mut self) {
-        self.drop.set(self.drop.get() + 1);
+        if let Some(drop_count) = self.drop {
+            drop_count.update(|c| c + 1);
+        }
     }
 }
 
 #[test]
 fn test_extend_from_within_clone() {
-    let clone_counts = [const { Cell::new(0) }; 6];
-    let drop_count = Cell::new(0);
+    let clone_counts = [const { Cell::new(0) }; 4];
     let mut v = VecDeque::with_capacity(10);
-    v.extend(clone_counts.iter().map(|clone_count| CloneTracker {
-        clone: clone_count,
-        drop: &drop_count,
+    // insert 2 dummy elements to have the buffer wrap later
+    v.extend([CloneTracker::DUMMY; 2]);
+    v.extend(clone_counts.iter().enumerate().map(|(id, clone_count)| CloneTracker {
+        id,
+        clone: Some(clone_count),
+        drop: None,
         panic: false,
     }));
+    // remove the dummy elements
     v.truncate_front(4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3]);
 
     v.extend_from_within(2..);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3, 2, 3]);
+    // elements at index 2 and 3 should have been cloned once
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [0, 0, 1, 1]);
+    // it is important that the deque wraps because of this operation, we want to test if wrapping is handled correctly
     v.extend_from_within(1..5);
-    assert_eq!(
-        v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(),
-        [0, 1, 3, 2, 3, 2, 1, 3, 2, 3],
-    );
-    assert_eq!(
-        clone_counts.each_ref().map(|c| {
-            let old = c.get();
-            c.set(0);
-            old
-        }),
-        [0, 0, 0, 1, 3, 2],
-    );
+    // total length is 10, 8 in the first part and 2 in the second part
+    assert_eq!(v.as_slices().0.len(), 8);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3, 2, 3, 1, 2, 3, 2]);
+    // the new elements are from indices 1, 2, 3 and 2, those elements should have their clone count
+    // incremented (clone count at index 2 gets incremented twice so ends up at 3)
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [0, 1, 3, 2]);
 }
 
 #[test]
 #[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
 fn test_extend_from_within_clone_panic() {
-    let clone_counts = [const { Cell::new(0) }; 6];
+    let clone_counts = [const { Cell::new(0) }; 4];
     let drop_count = Cell::new(0);
     let mut v = VecDeque::with_capacity(8);
-    v.extend(clone_counts.iter().map(|clone_count| CloneTracker {
-        clone: clone_count,
-        drop: &drop_count,
+    // insert 2 dummy elements to have the buffer wrap later
+    v.extend([CloneTracker::DUMMY; 2]);
+    v.extend(clone_counts.iter().enumerate().map(|(id, clone_count)| CloneTracker {
+        id,
+        clone: Some(clone_count),
+        drop: Some(&drop_count),
         panic: false,
     }));
-
+    // remove the dummy elements
     v.truncate_front(4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3]);
 
     // panic after wrapping
     v[2].panic = true;
@@ -1935,11 +1957,15 @@ fn test_extend_from_within_clone_panic() {
     }))
     .unwrap_err();
     v[2].panic = false;
-    assert_eq!(v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(), [1, 1, 1, 0, 1, 1]);
-    assert_eq!(clone_counts.each_ref().map(|c| c.get()), [0, 0, 1, 1, 1, 0]);
-    assert_eq!(drop_count.get(), 2);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3, 0, 1]);
+    // the first 2 elements were cloned
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [1, 1, 0, 0]);
+    // nothing should have been dropped
+    assert_eq!(drop_count.get(), 0);
 
     v.truncate_front(2);
+    assert_eq!(drop_count.get(), 4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1]);
 
     // panic before wrapping
     v[1].panic = true;
@@ -1948,9 +1974,11 @@ fn test_extend_from_within_clone_panic() {
     }))
     .unwrap_err();
     v[1].panic = false;
-    assert_eq!(v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(), [2, 2, 2]);
-    assert_eq!(clone_counts.each_ref().map(|c| c.get()), [0, 0, 2, 2, 1, 0]);
-    assert_eq!(drop_count.get(), 6);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 0]);
+    // only the first element was cloned
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [2, 1, 0, 0]);
+    // nothing more should have been dropped
+    assert_eq!(drop_count.get(), 4);
 }
 
 #[test]
@@ -1968,45 +1996,46 @@ fn test_prepend_from_within() {
 
 #[test]
 fn test_prepend_from_within_clone() {
-    let clone_counts = [const { Cell::new(0) }; 6];
-    let drop_count = Cell::new(0);
+    let clone_counts = [const { Cell::new(0) }; 4];
+    // insert 2 dummy elements to have the buffer wrap later
     let mut v = VecDeque::with_capacity(10);
-    v.extend(clone_counts.iter().map(|clone_count| CloneTracker {
-        clone: clone_count,
-        drop: &drop_count,
+    v.extend([CloneTracker::DUMMY; 2]);
+    v.extend(clone_counts.iter().enumerate().map(|(id, clone_count)| CloneTracker {
+        id,
+        clone: Some(clone_count),
+        drop: None,
         panic: false,
     }));
+    // remove the dummy elements
     v.truncate_front(4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3]);
 
     v.prepend_from_within(..2);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 0, 1, 2, 3]);
     v.prepend_from_within(1..5);
-    assert_eq!(
-        v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(),
-        [3, 2, 3, 1, 2, 3, 2, 3, 1, 0],
-    );
-    assert_eq!(
-        clone_counts.each_ref().map(|c| {
-            let old = c.get();
-            c.set(0);
-            old
-        }),
-        [0, 0, 2, 3, 1, 0],
-    );
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [1, 0, 1, 2, 0, 1, 0, 1, 2, 3]);
+    // count the number of each element and subtract one (clone should have been called n-1 times if we have n elements)
+    // example: 0 appears 3 times so should have been cloned twice, 1 appears 4 times so cloned 3 times, etc
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [2, 3, 1, 0]);
 }
 
 #[test]
 #[cfg_attr(not(panic = "unwind"), ignore = "test requires unwinding support")]
 fn test_prepend_from_within_clone_panic() {
-    let clone_counts = [const { Cell::new(0) }; 6];
+    let clone_counts = [const { Cell::new(0) }; 4];
     let drop_count = Cell::new(0);
     let mut v = VecDeque::with_capacity(8);
-    v.extend(clone_counts.iter().map(|clone_count| CloneTracker {
-        clone: clone_count,
-        drop: &drop_count,
+    // insert 2 dummy elements to have the buffer wrap later
+    v.extend([CloneTracker::DUMMY; 2]);
+    v.extend(clone_counts.iter().enumerate().map(|(id, clone_count)| CloneTracker {
+        id,
+        clone: Some(clone_count),
+        drop: Some(&drop_count),
         panic: false,
     }));
-
+    // remove the dummy elements
     v.truncate_front(4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [0, 1, 2, 3]);
 
     // panic after wrapping
     v[1].panic = true;
@@ -2015,11 +2044,15 @@ fn test_prepend_from_within_clone_panic() {
     }))
     .unwrap_err();
     v[1].panic = false;
-    assert_eq!(v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(), [1, 1, 0, 1, 1, 1]);
-    assert_eq!(clone_counts.each_ref().map(|c| c.get()), [0, 0, 0, 1, 1, 1]);
-    assert_eq!(drop_count.get(), 2);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [2, 3, 0, 1, 2, 3]);
+    // the last 2 elements were cloned
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [0, 0, 1, 1]);
+    // nothing should have been dropped
+    assert_eq!(drop_count.get(), 0);
 
     v.truncate_front(2);
+    assert_eq!(drop_count.get(), 4);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [2, 3]);
 
     // panic before wrapping
     v[0].panic = true;
@@ -2028,9 +2061,11 @@ fn test_prepend_from_within_clone_panic() {
     }))
     .unwrap_err();
     v[0].panic = false;
-    assert_eq!(v.iter().map(|tr| tr.clone.get()).collect::<Vec<_>>(), [2, 2, 2]);
-    assert_eq!(clone_counts.each_ref().map(|c| c.get()), [0, 0, 0, 1, 2, 2]);
-    assert_eq!(drop_count.get(), 6);
+    assert_eq!(v.iter().map(|tr| tr.id).collect::<Vec<_>>(), [3, 2, 3]);
+    // only the first element was cloned
+    assert_eq!(clone_counts.each_ref().map(Cell::get), [0, 0, 1, 2]);
+    // nothing more should have been dropped
+    assert_eq!(drop_count.get(), 4);
 }
 
 #[test]
@@ -2041,7 +2076,7 @@ fn test_extend_and_prepend_from_within() {
     v.prepend_from_within(..2);
     assert_eq!(v.iter().map(|s| &**s).collect::<String>(), "56567899");
     v.clear();
-    v.extend(['1', '2', '3'].into_iter().map(String::from));
+    v.extend(['1', '2', '3'].map(String::from));
     v.prepend_from_within(..);
     v.extend_from_within(..);
     assert_eq!(v.iter().map(|s| &**s).collect::<String>(), "123123123123");
