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.

Sign up for GitHub

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

Fix VecDeque::shrink_to and add tests. #108475

Merged
merged 2 commits into from
Feb 26, 2023
+104 −55
Merged

Fix VecDeque::shrink_to and add tests. #108475

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9e22516
Fix `VecDeque::shrink_to` and add tests.
Sp00ph Feb 26, 2023
4a4f43e
Disambiguate comments
Sp00ph Feb 26, 2023
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
117 changes: 62 additions & 55 deletions library/alloc/src/collections/vec_deque/mod.rs
View file
Open in desktop
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() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, heh, guess this wasn't ever doing anything, after the check on 943.

// 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
Open in desktop
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