summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGravatar Arthur O'Dwyer <arthur.j.odwyer@gmail.com>2024-03-03 18:47:41 -0800
committerGravatar Copybara-Service <copybara-worker@google.com>2024-03-03 18:48:47 -0800
commit7bd9ff910d489658da58251de1317eb3f790a2c6 (patch)
tree554dff6206f81d6e788f4fbd10e629b3d89f1553
parent7a4344511816e82234700795e7f2aaa80e85a119 (diff)
PR #1632: inlined_vector: Use trivial relocation for `erase`
-rw-r--r--absl/container/inlined_vector.h13
-rw-r--r--absl/container/inlined_vector_test.cc51
-rw-r--r--absl/container/internal/inlined_vector.h30
3 files changed, 86 insertions, 8 deletions
diff --git a/absl/container/inlined_vector.h b/absl/container/inlined_vector.h
index 04e2c385..974b6521 100644
--- a/absl/container/inlined_vector.h
+++ b/absl/container/inlined_vector.h
@@ -775,7 +775,20 @@ class InlinedVector {
ABSL_HARDENING_ASSERT(pos >= begin());
ABSL_HARDENING_ASSERT(pos < end());
+ // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102329#c2
+ // It appears that GCC thinks that since `pos` is a const pointer and may
+ // point to uninitialized memory at this point, a warning should be
+ // issued. But `pos` is actually only used to compute an array index to
+ // write to.
+#if !defined(__clang__) && defined(__GNUC__)
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wmaybe-uninitialized"
+#pragma GCC diagnostic ignored "-Wuninitialized"
+#endif
return storage_.Erase(pos, pos + 1);
+#if !defined(__clang__) && defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
}
// Overload of `InlinedVector::erase(...)` that erases every element in the
diff --git a/absl/container/inlined_vector_test.cc b/absl/container/inlined_vector_test.cc
index 5ecf88a9..6954262e 100644
--- a/absl/container/inlined_vector_test.cc
+++ b/absl/container/inlined_vector_test.cc
@@ -333,6 +333,57 @@ TEST(UniquePtr, Swap) {
}
}
+// Erasing from a container of unique pointers should work fine, with no
+// leaks, despite the fact that unique pointers are trivially relocatable but
+// not trivially destructible.
+// TODO(absl-team): Using unique_ptr here is technically correct, but
+// a trivially relocatable struct would be less semantically confusing.
+TEST(UniquePtr, EraseSingle) {
+ for (size_t size = 4; size < 16; ++size) {
+ absl::InlinedVector<std::unique_ptr<size_t>, 8> a;
+ for (size_t i = 0; i < size; ++i) {
+ a.push_back(std::make_unique<size_t>(i));
+ }
+ a.erase(a.begin());
+ ASSERT_THAT(a, SizeIs(size - 1));
+ for (size_t i = 0; i < size - 1; ++i) {
+ ASSERT_THAT(a[i], Pointee(i + 1));
+ }
+ a.erase(a.begin() + 2);
+ ASSERT_THAT(a, SizeIs(size - 2));
+ ASSERT_THAT(a[0], Pointee(1));
+ ASSERT_THAT(a[1], Pointee(2));
+ for (size_t i = 2; i < size - 2; ++i) {
+ ASSERT_THAT(a[i], Pointee(i + 2));
+ }
+ }
+}
+
+// Erasing from a container of unique pointers should work fine, with no
+// leaks, despite the fact that unique pointers are trivially relocatable but
+// not trivially destructible.
+// TODO(absl-team): Using unique_ptr here is technically correct, but
+// a trivially relocatable struct would be less semantically confusing.
+TEST(UniquePtr, EraseMulti) {
+ for (size_t size = 5; size < 16; ++size) {
+ absl::InlinedVector<std::unique_ptr<size_t>, 8> a;
+ for (size_t i = 0; i < size; ++i) {
+ a.push_back(std::make_unique<size_t>(i));
+ }
+ a.erase(a.begin(), a.begin() + 2);
+ ASSERT_THAT(a, SizeIs(size - 2));
+ for (size_t i = 0; i < size - 2; ++i) {
+ ASSERT_THAT(a[i], Pointee(i + 2));
+ }
+ a.erase(a.begin() + 1, a.begin() + 3);
+ ASSERT_THAT(a, SizeIs(size - 4));
+ ASSERT_THAT(a[0], Pointee(2));
+ for (size_t i = 1; i < size - 4; ++i) {
+ ASSERT_THAT(a[i], Pointee(i + 4));
+ }
+ }
+}
+
// At the end of this test loop, the elements between [erase_begin, erase_end)
// should have reference counts == 0, and all others elements should have
// reference counts == 1.
diff --git a/absl/container/internal/inlined_vector.h b/absl/container/internal/inlined_vector.h
index 90a74dc7..a1575328 100644
--- a/absl/container/internal/inlined_vector.h
+++ b/absl/container/internal/inlined_vector.h
@@ -893,16 +893,30 @@ auto Storage<T, N, A>::Erase(ConstIterator<A> from,
std::distance(ConstIterator<A>(storage_view.data), from));
SizeType<A> erase_end_index = erase_index + erase_size;
- IteratorValueAdapter<A, MoveIterator<A>> move_values(
- MoveIterator<A>(storage_view.data + erase_end_index));
-
- AssignElements<A>(storage_view.data + erase_index, move_values,
- storage_view.size - erase_end_index);
+ // Fast path: if the value type is trivially relocatable and we know
+ // the allocator doesn't do anything fancy, then we know it is legal for us to
+ // simply destroy the elements in the "erasure window" (which cannot throw)
+ // and then memcpy downward to close the window.
+ if (absl::is_trivially_relocatable<ValueType<A>>::value &&
+ std::is_nothrow_destructible<ValueType<A>>::value &&
+ std::is_same<A, std::allocator<ValueType<A>>>::value) {
+ DestroyAdapter<A>::DestroyElements(
+ GetAllocator(), storage_view.data + erase_index, erase_size);
+ std::memmove(
+ reinterpret_cast<char*>(storage_view.data + erase_index),
+ reinterpret_cast<const char*>(storage_view.data + erase_end_index),
+ (storage_view.size - erase_end_index) * sizeof(ValueType<A>));
+ } else {
+ IteratorValueAdapter<A, MoveIterator<A>> move_values(
+ MoveIterator<A>(storage_view.data + erase_end_index));
- DestroyAdapter<A>::DestroyElements(
- GetAllocator(), storage_view.data + (storage_view.size - erase_size),
- erase_size);
+ AssignElements<A>(storage_view.data + erase_index, move_values,
+ storage_view.size - erase_end_index);
+ DestroyAdapter<A>::DestroyElements(
+ GetAllocator(), storage_view.data + (storage_view.size - erase_size),
+ erase_size);
+ }
SubtractSize(erase_size);
return Iterator<A>(storage_view.data + erase_index);
}