Skip to content

Commit

Permalink
circular_deque: Fix erasing empty ranges.
Browse files Browse the repository at this point in the history
Previously circular_deque's range erase function would DCHECK if the caller
erased an empty range, because it would try to move the items on top of
themselves to make room for nothing.

This patch early exits in the erase-nothing case.

Change-Id: I772d1fcfaf5a85b1c79656274e44eac960a9c6e1
Reviewed-on: https://chromium-review.googlesource.com/646529
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Brett Wilson <brettw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499048}
  • Loading branch information
Brett Wilson authored and Commit Bot committed Aug 31, 2017
1 parent 62ff2e0 commit ba08f5c
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 5 deletions.
10 changes: 7 additions & 3 deletions base/containers/circular_deque.h
Original file line number Diff line number Diff line change
Expand Up @@ -772,8 +772,14 @@ class circular_deque {
ValidateIterator(first);
ValidateIterator(last);

IncrementGeneration();

// First, call the destructor on the deleted items.
if (first.index_ < last.index_) {
if (first.index_ == last.index_) {
// Nothing deleted. Need to return early to avoid falling through to
// moving items on top of themselves.
return iterator(this, first.index_);
} else if (first.index_ < last.index_) {
// Contiguous range.
buffer_.DestructRange(&buffer_[first.index_], &buffer_[last.index_]);
} else {
Expand All @@ -783,8 +789,6 @@ class circular_deque {
buffer_.DestructRange(&buffer_[0], &buffer_[last.index_]);
}

IncrementGeneration();

if (last.index_ == begin_) {
// This deletion is from the beginning. Nothing needs to be copied, only
// begin_ needs to be updated.
Expand Down
16 changes: 14 additions & 2 deletions base/containers/circular_deque_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,9 @@ TEST(CircularDeque, InsertFill) {
TEST(CircularDeque, InsertEraseRange) {
circular_deque<int> q;

// Erase nothing from an empty deque should work.
q.erase(q.begin(), q.end());

// Loop index used below to shift the used items in the buffer.
for (int i = 0; i < 10; i++) {
circular_deque<int> source;
Expand Down Expand Up @@ -768,11 +771,20 @@ TEST(CircularDeque, InsertEraseRange) {
EXPECT_EQ(1, q[6]);
EXPECT_EQ(2, q[7]);

// Now erase the inserted ranges.
auto result = q.erase(q.begin(), q.begin() + 2);
// Now erase the inserted ranges. Try each subsection also with no items
// being erased, which should be a no-op.
auto result = q.erase(q.begin(), q.begin()); // No-op.
EXPECT_EQ(q.begin(), result);
result = q.erase(q.begin(), q.begin() + 2);
EXPECT_EQ(q.begin(), result);

result = q.erase(q.begin() + 1, q.begin() + 1); // No-op.
EXPECT_EQ(q.begin() + 1, result);
result = q.erase(q.begin() + 1, q.begin() + 3);
EXPECT_EQ(q.begin() + 1, result);

result = q.erase(q.end() - 2, q.end() - 2); // No-op.
EXPECT_EQ(q.end() - 2, result);
result = q.erase(q.end() - 2, q.end());
EXPECT_EQ(q.end(), result);

Expand Down

0 comments on commit ba08f5c

Please sign in to comment.