Skip to content

Commit

Permalink
PR #1790: Respect the allocator's .destroy method in ~InlinedVector
Browse files Browse the repository at this point in the history
Imported from GitHub PR #1790

InlinedVector goes out of its way to use `A::construct` to construct new elements, so it should not skip the `A::destroy` call for those elements, either. Most codepaths seem fine (I didn't exhaustively explore this), but `DestroyAdapter` specifically failed to check whether the allocator had a non-trivial `destroy` method: it checked only whether the element type was trivially destructible in the absence of any `destroy` method.

Use the same condition in `DestroyAdapter` as we already use in the special short-circuit case on line 357 and in the conditions for the relocation optimizations on lines 304 and 324.

Unit test updated by @derekmauro

Merging this change closes #1790

COPYBARA_INTEGRATE_REVIEW=#1790 from Quuxplusone:inlined-vector-destroy 2c52d75
PiperOrigin-RevId: 705581176
Change-Id: Ic728747c41401863f8c01083e22b1d1e536347db
  • Loading branch information
Quuxplusone authored and copybara-github committed Dec 12, 2024
1 parent c935131 commit 940e0ec
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 29 deletions.
73 changes: 46 additions & 27 deletions absl/container/inlined_vector_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1749,61 +1749,80 @@ TEST(AllocatorSupportTest, CountAllocations) {
using MyAlloc = CountingAllocator<int>;
using AllocVec = absl::InlinedVector<int, 4, MyAlloc>;
const int ia[] = {0, 1, 2, 3, 4, 5, 6, 7};
int64_t allocated = 0;
MyAlloc alloc(&allocated);
int64_t bytes_allocated = 0;
int64_t instance_count = 0;
MyAlloc alloc(&bytes_allocated, &instance_count);
{
AllocVec ABSL_ATTRIBUTE_UNUSED v(ia, ia + 4, alloc);
EXPECT_THAT(allocated, Eq(0));
EXPECT_THAT(bytes_allocated, Eq(0));
EXPECT_THAT(instance_count, Eq(4));
}
EXPECT_THAT(allocated, Eq(0));
EXPECT_THAT(bytes_allocated, Eq(0));
EXPECT_THAT(instance_count, Eq(0));
{
AllocVec ABSL_ATTRIBUTE_UNUSED v(ia, ia + ABSL_ARRAYSIZE(ia), alloc);
EXPECT_THAT(allocated, Eq(static_cast<int64_t>(v.size() * sizeof(int))));
EXPECT_THAT(bytes_allocated,
Eq(static_cast<int64_t>(v.size() * sizeof(int))));
EXPECT_THAT(instance_count, Eq(static_cast<int64_t>(v.size())));
}
EXPECT_THAT(allocated, Eq(0));
EXPECT_THAT(bytes_allocated, Eq(0));
EXPECT_THAT(instance_count, Eq(0));
{
AllocVec v(4, 1, alloc);
EXPECT_THAT(allocated, Eq(0));
EXPECT_THAT(bytes_allocated, Eq(0));
EXPECT_THAT(instance_count, Eq(4));

int64_t allocated2 = 0;
MyAlloc alloc2(&allocated2);
int64_t bytes_allocated2 = 0;
MyAlloc alloc2(&bytes_allocated2);
ABSL_ATTRIBUTE_UNUSED AllocVec v2(v, alloc2);
EXPECT_THAT(allocated2, Eq(0));
EXPECT_THAT(bytes_allocated2, Eq(0));

int64_t allocated3 = 0;
MyAlloc alloc3(&allocated3);
int64_t bytes_allocated3 = 0;
MyAlloc alloc3(&bytes_allocated3);
ABSL_ATTRIBUTE_UNUSED AllocVec v3(std::move(v), alloc3);
EXPECT_THAT(allocated3, Eq(0));
EXPECT_THAT(bytes_allocated3, Eq(0));
}
EXPECT_THAT(allocated, 0);
EXPECT_THAT(bytes_allocated, Eq(0));
EXPECT_THAT(instance_count, Eq(0));
{
AllocVec v(8, 2, alloc);
EXPECT_THAT(allocated, Eq(static_cast<int64_t>(v.size() * sizeof(int))));
EXPECT_THAT(bytes_allocated,
Eq(static_cast<int64_t>(v.size() * sizeof(int))));
EXPECT_THAT(instance_count, Eq(static_cast<int64_t>(v.size())));

int64_t allocated2 = 0;
MyAlloc alloc2(&allocated2);
int64_t bytes_allocated2 = 0;
MyAlloc alloc2(&bytes_allocated2);
AllocVec v2(v, alloc2);
EXPECT_THAT(allocated2, Eq(static_cast<int64_t>(v2.size() * sizeof(int))));
EXPECT_THAT(bytes_allocated2,
Eq(static_cast<int64_t>(v2.size() * sizeof(int))));

int64_t allocated3 = 0;
MyAlloc alloc3(&allocated3);
int64_t bytes_allocated3 = 0;
MyAlloc alloc3(&bytes_allocated3);
AllocVec v3(std::move(v), alloc3);
EXPECT_THAT(allocated3, Eq(static_cast<int64_t>(v3.size() * sizeof(int))));
EXPECT_THAT(bytes_allocated3,
Eq(static_cast<int64_t>(v3.size() * sizeof(int))));
}
EXPECT_EQ(allocated, 0);
EXPECT_EQ(bytes_allocated, 0);
EXPECT_EQ(instance_count, 0);
{
// Test shrink_to_fit deallocations.
AllocVec v(8, 2, alloc);
EXPECT_EQ(allocated, static_cast<int64_t>(8 * sizeof(int)));
EXPECT_EQ(bytes_allocated, static_cast<int64_t>(8 * sizeof(int)));
EXPECT_EQ(instance_count, 8);
v.resize(5);
EXPECT_EQ(allocated, static_cast<int64_t>(8 * sizeof(int)));
EXPECT_EQ(bytes_allocated, static_cast<int64_t>(8 * sizeof(int)));
EXPECT_EQ(instance_count, 5);
v.shrink_to_fit();
EXPECT_EQ(allocated, static_cast<int64_t>(5 * sizeof(int)));
EXPECT_EQ(bytes_allocated, static_cast<int64_t>(5 * sizeof(int)));
EXPECT_EQ(instance_count, 5);
v.resize(4);
EXPECT_EQ(allocated, static_cast<int64_t>(5 * sizeof(int)));
EXPECT_EQ(bytes_allocated, static_cast<int64_t>(5 * sizeof(int)));
EXPECT_EQ(instance_count, 4);
v.shrink_to_fit();
EXPECT_EQ(allocated, 0);
EXPECT_EQ(bytes_allocated, 0);
EXPECT_EQ(instance_count, 4);
}
EXPECT_EQ(instance_count, 0);
}

TEST(AllocatorSupportTest, SwapBothAllocated) {
Expand Down
6 changes: 4 additions & 2 deletions absl/container/internal/inlined_vector.h
Original file line number Diff line number Diff line change
Expand Up @@ -83,8 +83,10 @@ using IsMoveAssignOk = std::is_move_assignable<ValueType<A>>;
template <typename A>
using IsSwapOk = absl::type_traits_internal::IsSwappable<ValueType<A>>;

template <typename A, bool IsTriviallyDestructible =
absl::is_trivially_destructible<ValueType<A>>::value>
template <typename A,
bool IsTriviallyDestructible =
absl::is_trivially_destructible<ValueType<A>>::value &&
std::is_same<A, std::allocator<ValueType<A>>>::value>
struct DestroyAdapter;

template <typename A>
Expand Down

0 comments on commit 940e0ec

Please sign in to comment.