From 90965f4c9662a73f2eb9c345b3a5431f40fd86d3 Mon Sep 17 00:00:00 2001 From: Abseil Team Date: Wed, 12 Oct 2022 05:52:24 -0700 Subject: `absl::InlinedVector` supports move assignment with non-assignable types. PiperOrigin-RevId: 480601268 Change-Id: I5a639da57b79ae600387c81e662d5c1542b2bf99 --- absl/container/BUILD.bazel | 1 + absl/container/CMakeLists.txt | 1 + absl/container/inlined_vector.h | 61 +++++++++++++---- absl/container/inlined_vector_test.cc | 111 +++++++++++++++++++++++++++++++ absl/container/internal/inlined_vector.h | 3 + 5 files changed, 165 insertions(+), 12 deletions(-) (limited to 'absl/container') diff --git a/absl/container/BUILD.bazel b/absl/container/BUILD.bazel index 6b4cedd2..d8dad8e8 100644 --- a/absl/container/BUILD.bazel +++ b/absl/container/BUILD.bazel @@ -134,6 +134,7 @@ cc_library( "//absl/base:core_headers", "//absl/base:throw_delegate", "//absl/memory", + "//absl/meta:type_traits", ], ) diff --git a/absl/container/CMakeLists.txt b/absl/container/CMakeLists.txt index 6c2931b6..25216466 100644 --- a/absl/container/CMakeLists.txt +++ b/absl/container/CMakeLists.txt @@ -194,6 +194,7 @@ absl_cc_library( absl::inlined_vector_internal absl::throw_delegate absl::memory + absl::type_traits PUBLIC ) diff --git a/absl/container/inlined_vector.h b/absl/container/inlined_vector.h index 60f12460..10b1896b 100644 --- a/absl/container/inlined_vector.h +++ b/absl/container/inlined_vector.h @@ -52,6 +52,7 @@ #include "absl/base/port.h" #include "absl/container/internal/inlined_vector.h" #include "absl/memory/memory.h" +#include "absl/meta/type_traits.h" namespace absl { ABSL_NAMESPACE_BEGIN @@ -77,6 +78,8 @@ class InlinedVector { using MoveIterator = inlined_vector_internal::MoveIterator; template using IsMemcpyOk = inlined_vector_internal::IsMemcpyOk; + template + using IsMoveAssignOk = inlined_vector_internal::IsMoveAssignOk; template using IteratorValueAdapter = @@ -94,6 +97,15 @@ class InlinedVector { using DisableIfAtLeastForwardIterator = absl::enable_if_t< !inlined_vector_internal::IsAtLeastForwardIterator::value, int>; + struct MemcpyPolicy {}; + struct ElementwiseAssignPolicy {}; + struct ElementwiseConstructPolicy {}; + + using MoveAssignmentPolicy = absl::conditional_t< + IsMemcpyOk::value, MemcpyPolicy, + absl::conditional_t::value, ElementwiseAssignPolicy, + ElementwiseConstructPolicy>>; + public: using allocator_type = A; using value_type = inlined_vector_internal::ValueType; @@ -486,18 +498,7 @@ class InlinedVector { // unspecified state. InlinedVector& operator=(InlinedVector&& other) { if (ABSL_PREDICT_TRUE(this != std::addressof(other))) { - if (IsMemcpyOk::value || other.storage_.GetIsAllocated()) { - inlined_vector_internal::DestroyAdapter::DestroyElements( - storage_.GetAllocator(), data(), size()); - storage_.DeallocateIfAllocated(); - storage_.MemcpyFrom(other.storage_); - - other.storage_.SetInlinedSize(0); - } else { - storage_.Assign(IteratorValueAdapter>( - MoveIterator(other.storage_.GetInlinedData())), - other.size()); - } + MoveAssignment(MoveAssignmentPolicy{}, std::move(other)); } return *this; @@ -773,6 +774,42 @@ class InlinedVector { template friend H AbslHashValue(H h, const absl::InlinedVector& a); + void MoveAssignment(MemcpyPolicy, InlinedVector&& other) { + inlined_vector_internal::DestroyAdapter::DestroyElements( + storage_.GetAllocator(), data(), size()); + storage_.DeallocateIfAllocated(); + storage_.MemcpyFrom(other.storage_); + + other.storage_.SetInlinedSize(0); + } + + void MoveAssignment(ElementwiseAssignPolicy, InlinedVector&& other) { + if (other.storage_.GetIsAllocated()) { + MoveAssignment(MemcpyPolicy{}, std::move(other)); + } else { + storage_.Assign(IteratorValueAdapter>( + MoveIterator(other.storage_.GetInlinedData())), + other.size()); + } + } + + void MoveAssignment(ElementwiseConstructPolicy, InlinedVector&& other) { + if (other.storage_.GetIsAllocated()) { + MoveAssignment(MemcpyPolicy{}, std::move(other)); + } else { + inlined_vector_internal::DestroyAdapter::DestroyElements( + storage_.GetAllocator(), data(), size()); + storage_.DeallocateIfAllocated(); + + IteratorValueAdapter> other_values( + MoveIterator(other.storage_.GetInlinedData())); + inlined_vector_internal::ConstructElements( + storage_.GetAllocator(), storage_.GetInlinedData(), other_values, + other.storage_.GetSize()); + storage_.SetInlinedSize(other.storage_.GetSize()); + } + } + Storage storage_; }; diff --git a/absl/container/inlined_vector_test.cc b/absl/container/inlined_vector_test.cc index b872eb45..65ddbab6 100644 --- a/absl/container/inlined_vector_test.cc +++ b/absl/container/inlined_vector_test.cc @@ -16,12 +16,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include "gmock/gmock.h" @@ -49,6 +51,7 @@ using testing::ElementsAre; using testing::ElementsAreArray; using testing::Eq; using testing::Gt; +using testing::Pointwise; using testing::PrintToString; using IntVec = absl::InlinedVector; @@ -1824,4 +1827,112 @@ TEST(InlinedVectorTest, AbslHashValueWorks) { EXPECT_TRUE(absl::VerifyTypeImplementsAbslHashCorrectly(cases)); } +class MoveConstructibleOnlyInstance + : public absl::test_internal::BaseCountedInstance { + public: + explicit MoveConstructibleOnlyInstance(int x) : BaseCountedInstance(x) {} + MoveConstructibleOnlyInstance(MoveConstructibleOnlyInstance&& other) = + default; + MoveConstructibleOnlyInstance& operator=( + MoveConstructibleOnlyInstance&& other) = delete; +}; + +MATCHER(HasValue, "") { + return ::testing::get<0>(arg).value() == ::testing::get<1>(arg); +} + +TEST(MoveAssignment, NonAssignable) { + using X = MoveConstructibleOnlyInstance; + { + InstanceTracker tracker; + absl::InlinedVector inlined; + inlined.emplace_back(1); + absl::InlinedVector allocated; + allocated.emplace_back(1); + allocated.emplace_back(2); + allocated.emplace_back(3); + tracker.ResetCopiesMovesSwaps(); + + inlined = std::move(allocated); + // passed ownership of the allocated storage + EXPECT_EQ(tracker.moves(), 0); + EXPECT_EQ(tracker.live_instances(), 3); + + EXPECT_THAT(inlined, Pointwise(HasValue(), {1, 2, 3})); + } + + { + InstanceTracker tracker; + absl::InlinedVector inlined; + inlined.emplace_back(1); + absl::InlinedVector allocated; + allocated.emplace_back(1); + allocated.emplace_back(2); + allocated.emplace_back(3); + tracker.ResetCopiesMovesSwaps(); + + allocated = std::move(inlined); + // Moved elements + EXPECT_EQ(tracker.moves(), 1); + EXPECT_EQ(tracker.live_instances(), 1); + + EXPECT_THAT(allocated, Pointwise(HasValue(), {1})); + } + + { + InstanceTracker tracker; + absl::InlinedVector inlined_a; + inlined_a.emplace_back(1); + absl::InlinedVector inlined_b; + inlined_b.emplace_back(1); + tracker.ResetCopiesMovesSwaps(); + + inlined_a = std::move(inlined_b); + // Moved elements + EXPECT_EQ(tracker.moves(), 1); + EXPECT_EQ(tracker.live_instances(), 1); + + EXPECT_THAT(inlined_a, Pointwise(HasValue(), {1})); + } + + { + InstanceTracker tracker; + absl::InlinedVector allocated_a; + allocated_a.emplace_back(1); + allocated_a.emplace_back(2); + allocated_a.emplace_back(3); + absl::InlinedVector allocated_b; + allocated_b.emplace_back(4); + allocated_b.emplace_back(5); + allocated_b.emplace_back(6); + allocated_b.emplace_back(7); + tracker.ResetCopiesMovesSwaps(); + + allocated_a = std::move(allocated_b); + // passed ownership of the allocated storage + EXPECT_EQ(tracker.moves(), 0); + EXPECT_EQ(tracker.live_instances(), 4); + + EXPECT_THAT(allocated_a, Pointwise(HasValue(), {4, 5, 6, 7})); + } + + { + InstanceTracker tracker; + absl::InlinedVector v; + v.emplace_back(1); + v.emplace_back(2); + v.emplace_back(3); + + tracker.ResetCopiesMovesSwaps(); + + // Obfuscated in order to pass -Wself-move. + v = std::move(*std::addressof(v)); + // nothing happens + EXPECT_EQ(tracker.moves(), 0); + EXPECT_EQ(tracker.live_instances(), 3); + + EXPECT_THAT(v, Pointwise(HasValue(), {1, 2, 3})); + } +} + } // anonymous namespace diff --git a/absl/container/internal/inlined_vector.h b/absl/container/internal/inlined_vector.h index a56b7573..f623494c 100644 --- a/absl/container/internal/inlined_vector.h +++ b/absl/container/internal/inlined_vector.h @@ -83,6 +83,9 @@ using IsMemcpyOk = absl::is_trivially_copy_assignable>, absl::is_trivially_destructible>>; +template +using IsMoveAssignOk = std::is_move_assignable>; + template struct TypeIdentity { using type = T; -- cgit v1.2.3