From 5214f8bc57ed327235bcd949589e6da58b2c10ca Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 28 Nov 2018 14:00:40 -0800 Subject: Fix InlinedVector to use its elements' move and copy methods. --- src/core/lib/gprpp/inlined_vector.h | 44 +++++---- test/core/gprpp/inlined_vector_test.cc | 164 ++++++++++++++++++++++++++++++++- 2 files changed, 187 insertions(+), 21 deletions(-) diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h index 65c2b9634f..66dc751a56 100644 --- a/src/core/lib/gprpp/inlined_vector.h +++ b/src/core/lib/gprpp/inlined_vector.h @@ -100,10 +100,7 @@ class InlinedVector { void reserve(size_t capacity) { if (capacity > capacity_) { T* new_dynamic = static_cast(gpr_malloc(sizeof(T) * capacity)); - for (size_t i = 0; i < size_; ++i) { - new (&new_dynamic[i]) T(std::move(data()[i])); - data()[i].~T(); - } + move_elements(data(), new_dynamic, size_); gpr_free(dynamic_); dynamic_ = new_dynamic; capacity_ = capacity; @@ -131,13 +128,25 @@ class InlinedVector { size_--; } + size_t size() const { return size_; } + bool empty() const { return size_ == 0; } + + size_t capacity() const { return capacity_; } + + void clear() { + destroy_elements(); + init_data(); + } + + private: void copy_from(const InlinedVector& v) { - // if v is allocated, copy over the buffer. + // if v is allocated, make sure we have enough capacity. if (v.dynamic_ != nullptr) { reserve(v.capacity_); - memcpy(dynamic_, v.dynamic_, v.size_ * sizeof(T)); - } else { - memcpy(inline_, v.inline_, v.size_ * sizeof(T)); + } + // copy over elements + for (size_t i = 0; i < v.size_; ++i) { + new (&(data()[i])) T(v[i]); } // copy over metadata size_ = v.size_; @@ -145,11 +154,12 @@ class InlinedVector { } void move_from(InlinedVector& v) { - // if v is allocated, then we steal its buffer, else we copy it. + // if v is allocated, then we steal its dynamic array; otherwise, we + // move the elements individually. if (v.dynamic_ != nullptr) { dynamic_ = v.dynamic_; } else { - memcpy(inline_, v.inline_, v.size_ * sizeof(T)); + move_elements(v.data(), data(), v.size_); } // copy over metadata size_ = v.size_; @@ -158,17 +168,13 @@ class InlinedVector { v.init_data(); } - size_t size() const { return size_; } - bool empty() const { return size_ == 0; } - - size_t capacity() const { return capacity_; } - - void clear() { - destroy_elements(); - init_data(); + static void move_elements(T* src, T* dst, size_t num_elements) { + for (size_t i = 0; i < num_elements; ++i) { + new (&dst[i]) T(std::move(src[i])); + src[i].~T(); + } } - private: void init_data() { dynamic_ = nullptr; size_ = 0; diff --git a/test/core/gprpp/inlined_vector_test.cc b/test/core/gprpp/inlined_vector_test.cc index 73e0773b31..6abd2e7dfa 100644 --- a/test/core/gprpp/inlined_vector_test.cc +++ b/test/core/gprpp/inlined_vector_test.cc @@ -116,7 +116,7 @@ typedef InlinedVector IntVec8; const size_t kInlinedFillSize = kInlinedLength - 1; const size_t kAllocatedFillSize = kInlinedLength + 1; -TEST(InlinedVectorTest, CopyConstructerInlined) { +TEST(InlinedVectorTest, CopyConstructorInlined) { IntVec8 original; FillVector(&original, kInlinedFillSize); IntVec8 copy_constructed(original); @@ -125,7 +125,7 @@ TEST(InlinedVectorTest, CopyConstructerInlined) { } } -TEST(InlinedVectorTest, CopyConstructerAllocated) { +TEST(InlinedVectorTest, CopyConstructorAllocated) { IntVec8 original; FillVector(&original, kAllocatedFillSize); IntVec8 copy_constructed(original); @@ -264,6 +264,166 @@ TEST(InlinedVectorTest, MoveAssignmentAllocatedAllocated) { EXPECT_EQ(move_assigned.data(), old_data); } +// A copyable and movable value class, used to test that elements' copy +// and move methods are called correctly. +class Value { + public: + explicit Value(int v) : value_(MakeUnique(v)) {} + + // copyable + Value(const Value& v) { + value_ = MakeUnique(*v.value_); + copied_ = true; + } + Value& operator=(const Value& v) { + value_ = MakeUnique(*v.value_); + copied_ = true; + return *this; + } + + // movable + Value(Value&& v) { + value_ = std::move(v.value_); + moved_ = true; + } + Value& operator=(Value&& v) { + value_ = std::move(v.value_); + moved_ = true; + return *this; + } + + const UniquePtr& value() const { return value_; } + bool copied() const { return copied_; } + bool moved() const { return moved_; } + + private: + UniquePtr value_; + bool copied_ = false; + bool moved_ = false; +}; + +TEST(InlinedVectorTest, CopyConstructorCopiesElementsInlined) { + InlinedVector v1; + v1.emplace_back(3); + InlinedVector v2(v1); + EXPECT_EQ(v2.size(), 1UL); + EXPECT_EQ(*v2[0].value(), 3); + // Addresses should differ. + EXPECT_NE(v1[0].value().get(), v2[0].value().get()); + EXPECT_TRUE(v2[0].copied()); +} + +TEST(InlinedVectorTest, CopyConstructorCopiesElementsAllocated) { + InlinedVector v1; + v1.reserve(2); + v1.emplace_back(3); + v1.emplace_back(5); + InlinedVector v2(v1); + EXPECT_EQ(v2.size(), 2UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(*v2[1].value(), 5); + // Addresses should differ. + EXPECT_NE(v1[0].value().get(), v2[0].value().get()); + EXPECT_NE(v1[1].value().get(), v2[1].value().get()); + EXPECT_TRUE(v2[0].copied()); + EXPECT_TRUE(v2[1].copied()); +} + +TEST(InlinedVectorTest, CopyAssignmentCopiesElementsInlined) { + InlinedVector v1; + v1.emplace_back(3); + InlinedVector v2; + EXPECT_EQ(v2.size(), 0UL); + v2 = v1; + EXPECT_EQ(v2.size(), 1UL); + EXPECT_EQ(*v2[0].value(), 3); + // Addresses should differ. + EXPECT_NE(v1[0].value().get(), v2[0].value().get()); + EXPECT_TRUE(v2[0].copied()); +} + +TEST(InlinedVectorTest, CopyAssignmentCopiesElementsAllocated) { + InlinedVector v1; + v1.reserve(2); + v1.emplace_back(3); + v1.emplace_back(5); + InlinedVector v2; + EXPECT_EQ(v2.size(), 0UL); + v2 = v1; + EXPECT_EQ(v2.size(), 2UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(*v2[1].value(), 5); + // Addresses should differ. + EXPECT_NE(v1[0].value().get(), v2[0].value().get()); + EXPECT_NE(v1[1].value().get(), v2[1].value().get()); + EXPECT_TRUE(v2[0].copied()); + EXPECT_TRUE(v2[1].copied()); +} + +TEST(InlinedVectorTest, MoveConstructorMovesElementsInlined) { + InlinedVector v1; + v1.emplace_back(3); + int* addr = v1[0].value().get(); + InlinedVector v2(std::move(v1)); + EXPECT_EQ(v2.size(), 1UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(addr, v2[0].value().get()); + EXPECT_TRUE(v2[0].moved()); +} + +TEST(InlinedVectorTest, MoveConstructorMovesElementsAllocated) { + InlinedVector v1; + v1.reserve(2); + v1.emplace_back(3); + v1.emplace_back(5); + int* addr1 = v1[0].value().get(); + int* addr2 = v1[1].value().get(); + Value* data1 = v1.data(); + InlinedVector v2(std::move(v1)); + EXPECT_EQ(v2.size(), 2UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(*v2[1].value(), 5); + EXPECT_EQ(addr1, v2[0].value().get()); + EXPECT_EQ(addr2, v2[1].value().get()); + // In this case, elements won't be moved, because we have just stolen + // the underlying storage. + EXPECT_EQ(data1, v2.data()); +} + +TEST(InlinedVectorTest, MoveAssignmentMovesElementsInlined) { + InlinedVector v1; + v1.emplace_back(3); + int* addr = v1[0].value().get(); + InlinedVector v2; + EXPECT_EQ(v2.size(), 0UL); + v2 = std::move(v1); + EXPECT_EQ(v2.size(), 1UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(addr, v2[0].value().get()); + EXPECT_TRUE(v2[0].moved()); +} + +TEST(InlinedVectorTest, MoveAssignmentMovesElementsAllocated) { + InlinedVector v1; + v1.reserve(2); + v1.emplace_back(3); + v1.emplace_back(5); + int* addr1 = v1[0].value().get(); + int* addr2 = v1[1].value().get(); + Value* data1 = v1.data(); + InlinedVector v2; + EXPECT_EQ(v2.size(), 0UL); + v2 = std::move(v1); + EXPECT_EQ(v2.size(), 2UL); + EXPECT_EQ(*v2[0].value(), 3); + EXPECT_EQ(*v2[1].value(), 5); + EXPECT_EQ(addr1, v2[0].value().get()); + EXPECT_EQ(addr2, v2[1].value().get()); + // In this case, elements won't be moved, because we have just stolen + // the underlying storage. + EXPECT_EQ(data1, v2.data()); +} + TEST(InlinedVectorTest, PopBackInlined) { InlinedVector, 2> v; // Add two elements, pop one out -- cgit v1.2.3