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.

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

[libc++] Improve the tests for vector::erase #116265

Merged
merged 3 commits into from
Nov 18, 2024

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Nov 14, 2024

In particular, test everything with both a normal and a min_allocator, add tests for a few corner cases and add tests with types that are trivially relocatable. Also add tests that count the number of assignments performed by vector::erase, since that is mandated by the Standard.

This patch is a preparation for optimizing vector::erase.

@ldionne ldionne requested a review from a team as a code owner November 14, 2024 18:03
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Nov 14, 2024
@llvmbot
Copy link

llvmbot commented Nov 14, 2024

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

In particular, test everything with both a normal and a min_allocator, add tests for a few corner cases and add tests with types that are trivially relocatable. Also add tests that count the number of assignments performed by vector::erase, since that is mandated by the Standard.

This patch is a preparation for optimizing vector::erase.


Patch is 24.96 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/116265.diff

6 Files Affected:

  • (modified) libcxx/test/benchmarks/ContainerBenchmarks.h (+30)
  • (modified) libcxx/test/benchmarks/deque.bench.cpp (+11)
  • (modified) libcxx/test/benchmarks/vector_operations.bench.cpp (+10)
  • (added) libcxx/test/std/containers/sequences/vector/vector.modifiers/common.h (+86)
  • (modified) libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp (+81-115)
  • (modified) libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp (+143-126)
diff --git a/libcxx/test/benchmarks/ContainerBenchmarks.h b/libcxx/test/benchmarks/ContainerBenchmarks.h
index 742c848328604c..c76dc80edc79c5 100644
--- a/libcxx/test/benchmarks/ContainerBenchmarks.h
+++ b/libcxx/test/benchmarks/ContainerBenchmarks.h
@@ -11,6 +11,8 @@
 #define BENCHMARK_CONTAINER_BENCHMARKS_H
 
 #include <cassert>
+#include <iterator>
+#include <utility>
 
 #include "Utilities.h"
 #include "benchmark/benchmark.h"
@@ -149,6 +151,34 @@ void BM_EmplaceDuplicate(benchmark::State& st, Container c, GenInputs gen) {
   }
 }
 
+template <class Container, class GenInputs>
+void BM_erase_iter_in_middle(benchmark::State& st, Container, GenInputs gen) {
+  auto in = gen(st.range(0));
+  Container c(in.begin(), in.end());
+  assert(c.size() > 2);
+  for (auto _ : st) {
+    auto mid    = std::next(c.begin(), c.size() / 2);
+    auto tmp    = *mid;
+    auto result = c.erase(mid);  // erase an element in the middle
+    c.push_back(std::move(tmp)); // and then push it back at the end to avoid needing a new container
+    benchmark::DoNotOptimize(result);
+  }
+}
+
+template <class Container, class GenInputs>
+void BM_erase_iter_at_start(benchmark::State& st, Container, GenInputs gen) {
+  auto in = gen(st.range(0));
+  Container c(in.begin(), in.end());
+  assert(c.size() > 2);
+  for (auto _ : st) {
+    auto it     = c.begin();
+    auto tmp    = *it;
+    auto result = c.erase(it);   // erase the first element
+    c.push_back(std::move(tmp)); // and then push it back at the end to avoid needing a new container
+    benchmark::DoNotOptimize(result);
+  }
+}
+
 template <class Container, class GenInputs>
 void BM_Find(benchmark::State& st, Container c, GenInputs gen) {
   auto in = gen(st.range(0));
diff --git a/libcxx/test/benchmarks/deque.bench.cpp b/libcxx/test/benchmarks/deque.bench.cpp
index b8f3b76dd27ee6..ab0ba96b12ffca 100644
--- a/libcxx/test/benchmarks/deque.bench.cpp
+++ b/libcxx/test/benchmarks/deque.bench.cpp
@@ -9,6 +9,7 @@
 // UNSUPPORTED: c++03, c++11, c++14, c++17, c++20
 
 #include <deque>
+#include <string>
 
 #include "benchmark/benchmark.h"
 
@@ -41,4 +42,14 @@ BENCHMARK_CAPTURE(BM_ConstructFromRange, deque_size_t, std::deque<size_t>{}, get
 BENCHMARK_CAPTURE(BM_ConstructFromRange, deque_string, std::deque<std::string>{}, getRandomStringInputs)
     ->Arg(TestNumInputs);
 
+BENCHMARK_CAPTURE(BM_erase_iter_in_middle, deque_int, std::deque<int>{}, getRandomIntegerInputs<int>)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+BENCHMARK_CAPTURE(BM_erase_iter_in_middle, deque_string, std::deque<std::string>{}, getRandomStringInputs)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+
+BENCHMARK_CAPTURE(BM_erase_iter_at_start, deque_int, std::deque<int>{}, getRandomIntegerInputs<int>)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+BENCHMARK_CAPTURE(BM_erase_iter_at_start, deque_string, std::deque<std::string>{}, getRandomStringInputs)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+
 BENCHMARK_MAIN();
diff --git a/libcxx/test/benchmarks/vector_operations.bench.cpp b/libcxx/test/benchmarks/vector_operations.bench.cpp
index ce8ab233fc9817..1855861263324d 100644
--- a/libcxx/test/benchmarks/vector_operations.bench.cpp
+++ b/libcxx/test/benchmarks/vector_operations.bench.cpp
@@ -54,6 +54,16 @@ BENCHMARK_CAPTURE(BM_ConstructFromRange, vector_string, std::vector<std::string>
 
 BENCHMARK_CAPTURE(BM_Pushback_no_grow, vector_int, std::vector<int>{})->Arg(TestNumInputs);
 
+BENCHMARK_CAPTURE(BM_erase_iter_in_middle, vector_int, std::vector<int>{}, getRandomIntegerInputs<int>)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+BENCHMARK_CAPTURE(BM_erase_iter_in_middle, vector_string, std::vector<std::string>{}, getRandomStringInputs)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+
+BENCHMARK_CAPTURE(BM_erase_iter_at_start, vector_int, std::vector<int>{}, getRandomIntegerInputs<int>)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+BENCHMARK_CAPTURE(BM_erase_iter_at_start, vector_string, std::vector<std::string>{}, getRandomStringInputs)
+    ->Range(TestNumInputs, TestNumInputs * 10);
+
 template <class T>
 void bm_grow(benchmark::State& state) {
   for (auto _ : state) {
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/common.h b/libcxx/test/std/containers/sequences/vector/vector.modifiers/common.h
new file mode 100644
index 00000000000000..72cd47a50b2c0d
--- /dev/null
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/common.h
@@ -0,0 +1,86 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef TEST_STD_CONTAINERS_SEQUENCES_VECTOR_VECTOR_MODIFIERS_COMMON_H
+#define TEST_STD_CONTAINERS_SEQUENCES_VECTOR_VECTOR_MODIFIERS_COMMON_H
+
+#include "test_macros.h"
+
+#include <type_traits> // for __libcpp_is_trivially_relocatable
+
+#ifndef TEST_HAS_NO_EXCEPTIONS
+struct Throws {
+  Throws() : v_(0) {}
+  Throws(int v) : v_(v) {}
+  Throws(const Throws& rhs) : v_(rhs.v_) {
+    if (sThrows)
+      throw 1;
+  }
+  Throws(Throws&& rhs) : v_(rhs.v_) {
+    if (sThrows)
+      throw 1;
+  }
+  Throws& operator=(const Throws& rhs) {
+    v_ = rhs.v_;
+    return *this;
+  }
+  Throws& operator=(Throws&& rhs) {
+    v_ = rhs.v_;
+    return *this;
+  }
+  int v_;
+  static bool sThrows;
+};
+
+bool Throws::sThrows = false;
+#endif
+
+struct Tracker {
+  int copy_assignments = 0;
+  int move_assignments = 0;
+};
+
+struct TrackedAssignment {
+  Tracker* tracker_;
+  TEST_CONSTEXPR_CXX14 explicit TrackedAssignment(Tracker* tracker) : tracker_(tracker) {}
+
+  TrackedAssignment(TrackedAssignment const&) = default;
+  TrackedAssignment(TrackedAssignment&&)      = default;
+
+  TEST_CONSTEXPR_CXX14 TrackedAssignment& operator=(TrackedAssignment const&) {
+    tracker_->copy_assignments++;
+    return *this;
+  }
+  TEST_CONSTEXPR_CXX14 TrackedAssignment& operator=(TrackedAssignment&&) {
+    tracker_->move_assignments++;
+    return *this;
+  }
+};
+
+struct NonTriviallyRelocatable {
+  int value_;
+  TEST_CONSTEXPR NonTriviallyRelocatable() : value_(0) {}
+  TEST_CONSTEXPR explicit NonTriviallyRelocatable(int v) : value_(v) {}
+  TEST_CONSTEXPR NonTriviallyRelocatable(NonTriviallyRelocatable const& other) : value_(other.value_) {}
+  TEST_CONSTEXPR NonTriviallyRelocatable(NonTriviallyRelocatable&& other) : value_(other.value_) {}
+  TEST_CONSTEXPR_CXX14 NonTriviallyRelocatable& operator=(NonTriviallyRelocatable const& other) {
+    value_ = other.value_;
+    return *this;
+  }
+  TEST_CONSTEXPR_CXX14 NonTriviallyRelocatable& operator=(NonTriviallyRelocatable&& other) {
+    value_ = other.value_;
+    return *this;
+  }
+
+  TEST_CONSTEXPR_CXX14 friend bool operator==(NonTriviallyRelocatable const& a, NonTriviallyRelocatable const& b) {
+    return a.value_ == b.value_;
+  }
+};
+LIBCPP_STATIC_ASSERT(!std::__libcpp_is_trivially_relocatable<NonTriviallyRelocatable>::value, "");
+
+#endif // TEST_STD_CONTAINERS_SEQUENCES_VECTOR_VECTOR_MODIFIERS_COMMON_H
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
index 549f29a8f7ba11..4a089fd7a4c4fc 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
@@ -11,135 +11,80 @@
 // iterator erase(const_iterator position);
 
 #include <vector>
-#include <iterator>
 #include <cassert>
+#include <memory>
 
 #include "asan_testing.h"
+#include "common.h"
 #include "min_allocator.h"
 #include "MoveOnly.h"
 #include "test_macros.h"
 
-#ifndef TEST_HAS_NO_EXCEPTIONS
-struct Throws {
-  Throws() : v_(0) {}
-  Throws(int v) : v_(v) {}
-  Throws(const Throws& rhs) : v_(rhs.v_) {
-    if (sThrows)
-      throw 1;
-  }
-  Throws(Throws&& rhs) : v_(rhs.v_) {
-    if (sThrows)
-      throw 1;
-  }
-  Throws& operator=(const Throws& rhs) {
-    v_ = rhs.v_;
-    return *this;
-  }
-  Throws& operator=(Throws&& rhs) {
-    v_ = rhs.v_;
-    return *this;
-  }
-  int v_;
-  static bool sThrows;
-};
-
-bool Throws::sThrows = false;
-#endif
-
-TEST_CONSTEXPR_CXX20 bool tests() {
-  {
-    int a1[] = {1, 2, 3, 4, 5};
-    std::vector<int> l1(a1, a1 + 5);
-    l1.erase(l1.begin());
-    assert(is_contiguous_container_asan_correct(l1));
-    assert(l1 == std::vector<int>(a1 + 1, a1 + 5));
-  }
+template <template <class> class Allocator, class T>
+TEST_CONSTEXPR_CXX20 void tests() {
   {
-    int a1[] = {1, 2, 3, 4, 5};
-    int e1[] = {1, 3, 4, 5};
-    std::vector<int> l1(a1, a1 + 5);
-    l1.erase(l1.begin() + 1);
-    assert(is_contiguous_container_asan_correct(l1));
-    assert(l1 == std::vector<int>(e1, e1 + 4));
-  }
-  {
-    int a1[] = {1, 2, 3};
-    std::vector<int> l1(a1, a1 + 3);
-    std::vector<int>::const_iterator i = l1.begin();
-    assert(is_contiguous_container_asan_correct(l1));
-    ++i;
-    std::vector<int>::iterator j = l1.erase(i);
-    assert(l1.size() == 2);
-    assert(std::distance(l1.begin(), l1.end()) == 2);
-    assert(*j == 3);
-    assert(*l1.begin() == 1);
-    assert(*std::next(l1.begin()) == 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    j = l1.erase(j);
-    assert(j == l1.end());
-    assert(l1.size() == 1);
-    assert(std::distance(l1.begin(), l1.end()) == 1);
-    assert(*l1.begin() == 1);
-    assert(is_contiguous_container_asan_correct(l1));
-    j = l1.erase(l1.begin());
-    assert(j == l1.end());
-    assert(l1.size() == 0);
-    assert(std::distance(l1.begin(), l1.end()) == 0);
-    assert(is_contiguous_container_asan_correct(l1));
-  }
+    T arr[]             = {T(1), T(2), T(3), T(4), T(5)};
+    using Vector        = std::vector<T, Allocator<T> >;
+    using Iterator      = typename Vector::iterator;
+    using ConstIterator = typename Vector::const_iterator;
 
-  // Make sure vector::erase works with move-only types
-  // When non-trivial
-  {
-    std::vector<MoveOnly> v;
-    v.emplace_back(1);
-    v.emplace_back(2);
-    v.emplace_back(3);
-    v.erase(v.begin());
-    assert(v.size() == 2);
-    assert(v[0] == MoveOnly(2));
-    assert(v[1] == MoveOnly(3));
-  }
-  // When trivial
-  {
-    std::vector<TrivialMoveOnly> v;
-    v.emplace_back(1);
-    v.emplace_back(2);
-    v.emplace_back(3);
-    v.erase(v.begin());
-    assert(v.size() == 2);
-    assert(v[0] == TrivialMoveOnly(2));
-    assert(v[1] == TrivialMoveOnly(3));
+    {
+      Vector v(arr, arr + 5);
+      Iterator it = v.erase(v.cbegin());
+      assert(v == Vector(arr + 1, arr + 5));
+      assert(it == v.begin());
+      assert(is_contiguous_container_asan_correct(v));
+    }
+    {
+      T expected[] = {T(1), T(3), T(4), T(5)};
+      Vector v(arr, arr + 5);
+      Iterator it = v.erase(v.cbegin() + 1);
+      assert(v == Vector(expected, expected + 4));
+      assert(it == v.begin() + 1);
+      assert(is_contiguous_container_asan_correct(v));
+    }
+    {
+      T expected[] = {T(1), T(2), T(3), T(4)};
+      Vector v(arr, arr + 5);
+      Iterator it = v.erase(v.cbegin() + 4);
+      assert(v == Vector(expected, expected + 4));
+      assert(it == v.end());
+      assert(is_contiguous_container_asan_correct(v));
+    }
   }
 
-#if TEST_STD_VER >= 11
+  // Make sure vector::erase works with move-only types
   {
-    int a1[] = {1, 2, 3};
-    std::vector<int, min_allocator<int>> l1(a1, a1 + 3);
-    std::vector<int, min_allocator<int>>::const_iterator i = l1.begin();
-    assert(is_contiguous_container_asan_correct(l1));
-    ++i;
-    std::vector<int, min_allocator<int>>::iterator j = l1.erase(i);
-    assert(l1.size() == 2);
-    assert(std::distance(l1.begin(), l1.end()) == 2);
-    assert(*j == 3);
-    assert(*l1.begin() == 1);
-    assert(*std::next(l1.begin()) == 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    j = l1.erase(j);
-    assert(j == l1.end());
-    assert(l1.size() == 1);
-    assert(std::distance(l1.begin(), l1.end()) == 1);
-    assert(*l1.begin() == 1);
-    assert(is_contiguous_container_asan_correct(l1));
-    j = l1.erase(l1.begin());
-    assert(j == l1.end());
-    assert(l1.size() == 0);
-    assert(std::distance(l1.begin(), l1.end()) == 0);
-    assert(is_contiguous_container_asan_correct(l1));
+    // When non-trivial
+    {
+      std::vector<MoveOnly, Allocator<MoveOnly> > v;
+      v.emplace_back(1);
+      v.emplace_back(2);
+      v.emplace_back(3);
+      v.erase(v.begin());
+      assert(v.size() == 2);
+      assert(v[0] == MoveOnly(2));
+      assert(v[1] == MoveOnly(3));
+    }
+    // When trivial
+    {
+      std::vector<TrivialMoveOnly, Allocator<TrivialMoveOnly> > v;
+      v.emplace_back(1);
+      v.emplace_back(2);
+      v.emplace_back(3);
+      v.erase(v.begin());
+      assert(v.size() == 2);
+      assert(v[0] == TrivialMoveOnly(2));
+      assert(v[1] == TrivialMoveOnly(3));
+    }
   }
-#endif
+}
 
+TEST_CONSTEXPR_CXX20 bool tests() {
+  tests<std::allocator, int>();
+  tests<std::allocator, NonTriviallyRelocatable>();
+  tests<min_allocator, int>();
+  tests<min_allocator, NonTriviallyRelocatable>();
   return true;
 }
 
@@ -163,5 +108,26 @@ int main(int, char**) {
   }
 #endif
 
+  // Make sure we satisfy the complexity requirement in terms of the number of times the assignment
+  // operator is called.
+  {
+    Tracker tracker;
+    std::vector<TrackedAssignment> v;
+
+    // Set up the vector with 5 elements.
+    for (int i = 0; i != 5; ++i) {
+      v.emplace_back(&tracker);
+    }
+    assert(tracker.copy_assignments == 0);
+    assert(tracker.move_assignments == 0);
+
+    // Erase element [1] from it. Elements [2] [3] [4] should be shifted, so we should
+    // see 3 move assignments (and nothing else).
+    v.erase(v.begin() + 1);
+    assert(v.size() == 4);
+    assert(tracker.copy_assignments == 0);
+    assert(tracker.move_assignments == 3);
+  }
+
   return 0;
 }
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp
index 4091e71d814e38..f972034681ef38 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp
@@ -11,86 +11,107 @@
 // iterator erase(const_iterator first, const_iterator last);
 
 #include <vector>
-#include <iterator>
 #include <cassert>
+#include <memory>
+#include <string>
 
 #include "asan_testing.h"
+#include "common.h"
 #include "min_allocator.h"
 #include "MoveOnly.h"
 #include "test_macros.h"
 
-#ifndef TEST_HAS_NO_EXCEPTIONS
-struct Throws {
-  Throws() : v_(0) {}
-  Throws(int v) : v_(v) {}
-  Throws(const Throws& rhs) : v_(rhs.v_) {
-    if (sThrows)
-      throw 1;
-  }
-  Throws(Throws&& rhs) : v_(rhs.v_) {
-    if (sThrows)
-      throw 1;
-  }
-  Throws& operator=(const Throws& rhs) {
-    v_ = rhs.v_;
-    return *this;
-  }
-  Throws& operator=(Throws&& rhs) {
-    v_ = rhs.v_;
-    return *this;
-  }
-  int v_;
-  static bool sThrows;
-};
+template <template <class> class Allocator, class T>
+TEST_CONSTEXPR_CXX20 void tests() {
+  {
+    T arr[]             = {T(1), T(2), T(3)};
+    using Vector        = std::vector<T, Allocator<T> >;
+    using Iterator      = typename Vector::iterator;
+    using ConstIterator = typename Vector::const_iterator;
 
-bool Throws::sThrows = false;
-#endif
+    // Erase an empty range [first, last): last should be returned
+    {
+      {
+        Vector v;
+        Iterator i = v.erase(v.end(), v.end());
+        assert(v.empty());
+        assert(i == v.end());
+        assert(is_contiguous_container_asan_correct(v));
+      }
+      {
+        Vector v(arr, arr + 3);
+        ConstIterator first = v.cbegin(), last = v.cbegin();
+        Iterator i = v.erase(first, last);
+        assert(v == Vector(arr, arr + 3));
+        assert(i == last);
+        assert(is_contiguous_container_asan_correct(v));
+      }
+      {
+        Vector v(arr, arr + 3);
+        ConstIterator first = v.cbegin() + 1, last = v.cbegin() + 1;
+        Iterator i = v.erase(first, last);
+        assert(v == Vector(arr, arr + 3));
+        assert(i == last);
+        assert(is_contiguous_container_asan_correct(v));
+      }
+      {
+        Vector v(arr, arr + 3);
+        ConstIterator first = v.cbegin(), last = v.cbegin();
+        Iterator i = v.erase(first, last);
+        assert(v == Vector(arr, arr + 3));
+        assert(i == last);
+        assert(is_contiguous_container_asan_correct(v));
+      }
+    }
 
-TEST_CONSTEXPR_CXX20 bool tests() {
-  int a1[] = {1, 2, 3};
-  {
-    std::vector<int> l1(a1, a1 + 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    std::vector<int>::iterator i = l1.erase(l1.cbegin(), l1.cbegin());
-    assert(l1.size() == 3);
-    assert(std::distance(l1.cbegin(), l1.cend()) == 3);
-    assert(i == l1.begin());
-    assert(is_contiguous_container_asan_correct(l1));
-  }
-  {
-    std::vector<int> l1(a1, a1 + 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    std::vector<int>::iterator i = l1.erase(l1.cbegin(), std::next(l1.cbegin()));
-    assert(l1.size() == 2);
-    assert(std::distance(l1.cbegin(), l1.cend()) == 2);
-    assert(i == l1.begin());
-    assert(l1 == std::vector<int>(a1 + 1, a1 + 3));
-    assert(is_contiguous_container_asan_correct(l1));
-  }
-  {
-    std::vector<int> l1(a1, a1 + 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    std::vector<int>::iterator i = l1.erase(l1.cbegin(), std::next(l1.cbegin(), 2));
-    assert(l1.size() == 1);
-    assert(std::distance(l1.cbegin(), l1.cend()) == 1);
-    assert(i == l1.begin());
-    assert(l1 == std::vector<int>(a1 + 2, a1 + 3));
-    assert(is_contiguous_container_asan_correct(l1));
-  }
-  {
-    std::vector<int> l1(a1, a1 + 3);
-    assert(is_contiguous_container_asan_correct(l1));
-    std::vector<int>::iterator i = l1.erase(l1.cbegin(), std::next(l1.cbegin(), 3));
-    assert(l1.size() == 0);
-    assert(std::distance(l1.cbegin(), l1.cend()) == 0);
-    assert(i == l1.begin());
-    assert(is_contiguous_container_asan_correct(l1));
+    // Erase non-empty ranges
+    {
+      // Starting at begin()
+      {
+        {
+          Vector v(arr, arr + 3);
+          Iterator i = v.erase(v.cbegin(), v.cbegin() + 1);
+          assert(v == Vector(arr + 1, arr + 3));
+          assert(i == v.begin());
+          assert(is_contiguous_container_asan_correct(v));
+        }
+        {
+          Vector v(arr, arr + 3);
+          Iterator i = v.erase(v.cbegin(), v.cbegin() + 2);
+          assert(v == Vector(arr + 2, arr + 3));
+          assert(i == v.begin());
+          assert(is_contiguous_container_asan_correct(v));
+        }
+        {
+          Vector v(arr, arr + 3);
+          Iterator i = v.erase(v.cbegin(), v.end());
+          assert(v.size() == 0);
+          assert(i == v.begin());
+          assert(is_contiguous_container_asan_correct(v));
+        }
+      }
+      {
+        Vector v(arr, arr + 3);
+        Iterator i = v.erase(v.cbegin() + 1, v.cbegin() + 2);
+        assert(v.size() == 2);
+        assert(v[0] == arr[0]);
+        assert(v[1] == arr[2]);
+        assert(i == v.begin() + 1);
+        assert(is_contiguous_container_asan_correct(v));
+      }
+      {
+        Vector v(arr, arr + 3);
+        Iterator i = v.erase(v.cbegin() + 1, v.cend());
+        assert(v == Vector(arr, arr + 1));
+        assert(i == v.begin() + 1);
+        assert(is_contiguous_container_asan_correct(v));
+      }
+    }
   }
   {
-    std::vector<std::vector<int> > outer(2, std::vector<int>(1));
-    assert(is_contiguous_container_asan_correct(outer));
-    assert(is_contiguous_container_asan_correct(outer[0]));
-    assert(is_contiguous_container_asan_correct(outer[1]));
+    using InnerVector = std::vector<T, Allocator<T> >;
+    using Vector      = std::vector<InnerVector, Allocator<InnerVector> >;
+    Vector outer(2, InnerVector(1));
     outer.erase(outer.begin(), outer.begin());
     assert(outer.size() == 2);
     assert(outer[0].size() == 1);
@@ -99,11 +120,12 @@ TEST_CONSTEXPR_CXX20...
[truncated]

In particular, test everything with both a normal and a min_allocator,
add tests for a few corner cases and add tests with types that are
trivially relocatable. Also add tests that count the number of assignments
performed by vector::erase, since that is mandated by the Standard.

This patch is a preparation for optimizing vector::erase.
@ldionne ldionne force-pushed the review/improve-vector-erase-tests branch from 7f8c872 to 9938700 Compare November 18, 2024 12:36
Copy link

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 5a48162dc88e0c3db7bc0a63dee0eb3182ef00e3 869a6d5f8e243f78ae87ccf9690078efafd6d84f --extensions h,cpp -- libcxx/test/std/containers/sequences/vector/vector.modifiers/common.h libcxx/test/benchmarks/ContainerBenchmarks.h libcxx/test/benchmarks/deque.bench.cpp libcxx/test/benchmarks/vector_operations.bench.cpp libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter_iter.pass.cpp
View the diff from clang-format here.
diff --git a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
index f0157eb74b..0c65bd1034 100644
--- a/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
+++ b/libcxx/test/std/containers/sequences/vector/vector.modifiers/erase_iter.pass.cpp
@@ -23,9 +23,9 @@
 template <template <class> class Allocator, class T>
 TEST_CONSTEXPR_CXX20 void tests() {
   {
-    T arr[]             = {T(1), T(2), T(3), T(4), T(5)};
-    using Vector        = std::vector<T, Allocator<T> >;
-    using Iterator      = typename Vector::iterator;
+    T arr[]        = {T(1), T(2), T(3), T(4), T(5)};
+    using Vector   = std::vector<T, Allocator<T> >;
+    using Iterator = typename Vector::iterator;
 
     {
       Vector v(arr, arr + 5);

@ldionne ldionne merged commit 3a3517c into llvm:main Nov 18, 2024
56 of 61 checks passed
@ldionne ldionne deleted the review/improve-vector-erase-tests branch November 18, 2024 23:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants