Skip to content

Commit

Permalink
Rollup merge of #108475 - Sp00ph:fix_shrink_to, r=thomcc
Browse files Browse the repository at this point in the history 
Fix `VecDeque::shrink_to` and add tests.

Fixes #108453.

Also adds both a specific test with the code from #108453 and an exhaustive test that checks all possible head positions, lengths and target capacities for deques with capacity 16.

cc `@trinity-1686a` `@scottmcm`
  • Loading branch information
@matthiaskrgr
matthiaskrgr authored Feb 26, 2023
2 parents c815e03 + 4a4f43e commit be23b32
Show file tree
Hide file tree
Showing 2 changed files with 104 additions and 55 deletions.
117 changes: 62 additions & 55 deletions library/alloc/src/collections/vec_deque/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -944,65 +944,72 @@ impl<T, A: Allocator> VecDeque<T, A> {
return;
}

if target_cap < self.capacity() {
// There are three cases of interest:
// All elements are out of desired bounds
// Elements are contiguous, and head is out of desired bounds
// Elements are discontiguous, and tail is out of desired bounds
// There are three cases of interest:
// All elements are out of desired bounds
// Elements are contiguous, and tail is out of desired bounds
// Elements are discontiguous
//
// At all other times, element positions are unaffected.

// `head` and `len` are at most `isize::MAX` and `target_cap < self.capacity()`, so nothing can
// overflow.
let tail_outside = (target_cap + 1..=self.capacity()).contains(&(self.head + self.len));

if self.len == 0 {
self.head = 0;
} else if self.head >= target_cap && tail_outside {
// Head and tail are both out of bounds, so copy all of them to the front.
//
// At all other times, element positions are unaffected.
// H := head
// L := last element
// H L
// [. . . . . . . . o o o o o o o . ]
// H L
// [o o o o o o o . ]
unsafe {
// nonoverlapping because `self.head >= target_cap >= self.len`.
self.copy_nonoverlapping(self.head, 0, self.len);
}
self.head = 0;
} else if self.head < target_cap && tail_outside {
// Head is in bounds, tail is out of bounds.
// Copy the overflowing part to the beginning of the
// buffer. This won't overlap because `target_cap >= self.len`.
//
// Indicates that elements at the head should be moved.

let tail_outside = (target_cap + 1..=self.capacity()).contains(&(self.head + self.len));
// Move elements from out of desired bounds (positions after target_cap)
if self.len == 0 {
self.head = 0;
} else if self.head >= target_cap && tail_outside {
// H := head
// L := last element
// H L
// [. . . . . . . . o o o o o o o . ]
// H L
// [o o o o o o o . ]
unsafe {
// nonoverlapping because self.head >= target_cap >= self.len
self.copy_nonoverlapping(self.head, 0, self.len);
}
self.head = 0;
} else if self.head < target_cap && tail_outside {
// H := head
// L := last element
// H L
// [. . . o o o o o o o . . . . . . ]
// L H
// [o o . o o o o o ]
let len = self.head + self.len - target_cap;
unsafe {
self.copy_nonoverlapping(target_cap, 0, len);
}
} else if self.head >= target_cap {
// H := head
// L := last element
// L H
// [o o o o o . . . . . . . . . o o ]
// L H
// [o o o o o . o o ]
let len = self.capacity() - self.head;
let new_head = target_cap - len;
unsafe {
// can't use copy_nonoverlapping here for the same reason
// as in `handle_capacity_increase()`
self.copy(self.head, new_head, len);
}
self.head = new_head;
// H := head
// L := last element
// H L
// [. . . o o o o o o o . . . . . . ]
// L H
// [o o . o o o o o ]
let len = self.head + self.len - target_cap;
unsafe {
self.copy_nonoverlapping(target_cap, 0, len);
}

self.buf.shrink_to_fit(target_cap);

debug_assert!(self.head < self.capacity() || self.capacity() == 0);
debug_assert!(self.len <= self.capacity());
} else if !self.is_contiguous() {
// The head slice is at least partially out of bounds, tail is in bounds.
// Copy the head backwards so it lines up with the target capacity.
// This won't overlap because `target_cap >= self.len`.
//
// H := head
// L := last element
// L H
// [o o o o o . . . . . . . . . o o ]
// L H
// [o o o o o . o o ]
let head_len = self.capacity() - self.head;
let new_head = target_cap - head_len;
unsafe {
// can't use `copy_nonoverlapping()` here because the new and old
// regions for the head might overlap.
self.copy(self.head, new_head, head_len);
}
self.head = new_head;
}
self.buf.shrink_to_fit(target_cap);

debug_assert!(self.head < self.capacity() || self.capacity() == 0);
debug_assert!(self.len <= self.capacity());
}

/// Shortens the deque, keeping the first `len` elements and dropping
Expand Down
42 changes: 42 additions & 0 deletions library/alloc/src/collections/vec_deque/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -748,6 +748,48 @@ fn test_drain() {
}
}

#[test]
fn issue_108453() {
let mut deque = VecDeque::with_capacity(10);

deque.push_back(1u8);
deque.push_back(2);
deque.push_back(3);

deque.push_front(10);
deque.push_front(9);

deque.shrink_to(9);

assert_eq!(deque.into_iter().collect::<Vec<_>>(), vec![9, 10, 1, 2, 3]);
}

#[test]
fn test_shrink_to() {
// test deques with capacity 16 with all possible head positions, lengths and target capacities.
let cap = 16;

for len in 0..cap {
for head in 0..cap {
let expected = (1..=len).collect::<VecDeque<_>>();

for target_cap in len..cap {
let mut deque = VecDeque::with_capacity(cap);
// currently, `with_capacity` always allocates the exact capacity if it's greater than 8.
assert_eq!(deque.capacity(), cap);

// we can let the head point anywhere in the buffer since the deque is empty.
deque.head = head;
deque.extend(1..=len);

deque.shrink_to(target_cap);

assert_eq!(deque, expected);
}
}
}
}

#[test]
fn test_shrink_to_fit() {
// This test checks that every single combination of head and tail position,
Expand Down

0 comments on commit be23b32

Please sign in to comment.