From 2968bf687af0f5e0db591d20276b79a7fd627c31 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 17 Jul 2018 15:11:27 -0700 Subject: [PATCH] reviewer feedback --- src/core/lib/gprpp/inlined_vector.h | 2 +- test/core/gprpp/inlined_vector_test.cc | 229 +++++++++++-------------- 2 files changed, 98 insertions(+), 133 deletions(-) diff --git a/src/core/lib/gprpp/inlined_vector.h b/src/core/lib/gprpp/inlined_vector.h index 508fb2eed11..76e2f0a7850 100644 --- a/src/core/lib/gprpp/inlined_vector.h +++ b/src/core/lib/gprpp/inlined_vector.h @@ -124,7 +124,7 @@ class InlinedVector { void push_back(T&& value) { emplace_back(std::move(value)); } void copy_from(const InlinedVector& v) { - // if copy over the buffer from v. + // if v is allocated, copy over the buffer. if (v.dynamic_ != nullptr) { reserve(v.capacity_); memcpy(dynamic_, v.dynamic_, v.size_ * sizeof(T)); diff --git a/test/core/gprpp/inlined_vector_test.cc b/test/core/gprpp/inlined_vector_test.cc index 1ef7da465cc..e9d1eb2c763 100644 --- a/test/core/gprpp/inlined_vector_test.cc +++ b/test/core/gprpp/inlined_vector_test.cc @@ -32,6 +32,8 @@ static void FillVector(Vector* v, int len, int start = 0) { v->push_back(i + start); EXPECT_EQ(i + 1UL, v->size()); } + EXPECT_EQ(static_cast(len), v->size()); + EXPECT_LE(static_cast(len), v->capacity()); } } // namespace @@ -107,14 +109,16 @@ TEST(InlinedVectorTest, ConstIndexOperator) { const_func(v); } +// the following constants and typedefs are used for copy/move +// construction/assignment +const size_t kInlinedLength = 8; +typedef InlinedVector IntVec8; +const size_t kInlinedFillSize = kInlinedLength - 1; +const size_t kAllocatedFillSize = kInlinedLength + 1; + TEST(InlinedVectorTest, CopyConstructerInlined) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength - 1; - typedef InlinedVector IntVec8; IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); + FillVector(&original, kInlinedFillSize); IntVec8 copy_constructed(original); for (size_t i = 0; i < original.size(); ++i) { EXPECT_EQ(original[i], copy_constructed[i]); @@ -122,83 +126,61 @@ TEST(InlinedVectorTest, CopyConstructerInlined) { } TEST(InlinedVectorTest, CopyConstructerAllocated) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength + 1; - typedef InlinedVector IntVec8; IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); + FillVector(&original, kAllocatedFillSize); IntVec8 copy_constructed(original); for (size_t i = 0; i < original.size(); ++i) { EXPECT_EQ(original[i], copy_constructed[i]); } } -TEST(InlinedVectorTest, CopyAssignementInlined) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength - 1; - typedef InlinedVector IntVec8; +TEST(InlinedVectorTest, CopyAssignementInlinedInlined) { IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); - // copy assigned vector is inlined - { - IntVec8 copy_assigned; - FillVector(©_assigned, kInlinedLength - 1, 99); - copy_assigned = original; - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], copy_assigned[i]); - } + FillVector(&original, kInlinedFillSize); + IntVec8 copy_assigned; + FillVector(©_assigned, kInlinedFillSize, 99); + copy_assigned = original; + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], copy_assigned[i]); } - // copy assigned vector is allocated - { - IntVec8 copy_assigned; - FillVector(©_assigned, kInlinedLength + 1, 99); - copy_assigned = original; - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], copy_assigned[i]); - } +} + +TEST(InlinedVectorTest, CopyAssignementInlinedAllocated) { + IntVec8 original; + FillVector(&original, kInlinedFillSize); + IntVec8 copy_assigned; + FillVector(©_assigned, kAllocatedFillSize, 99); + copy_assigned = original; + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], copy_assigned[i]); } } -TEST(InlinedVectorTest, CopyAssignementAllocated) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength + 1; - typedef InlinedVector IntVec8; +TEST(InlinedVectorTest, CopyAssignementAllocatedInlined) { IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); - // copy assigned vector is inlined - { - IntVec8 copy_assigned; - FillVector(©_assigned, kInlinedLength - 1, 99); - copy_assigned = original; - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], copy_assigned[i]); - } + FillVector(&original, kAllocatedFillSize); + IntVec8 copy_assigned; + FillVector(©_assigned, kInlinedFillSize, 99); + copy_assigned = original; + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], copy_assigned[i]); } - // copy assigned vector is allocated - { - IntVec8 copy_assigned; - FillVector(©_assigned, kInlinedLength + 1, 99); - copy_assigned = original; - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], copy_assigned[i]); - } +} + +TEST(InlinedVectorTest, CopyAssignementAllocatedAllocated) { + IntVec8 original; + FillVector(&original, kAllocatedFillSize); + IntVec8 copy_assigned; + FillVector(©_assigned, kAllocatedFillSize, 99); + copy_assigned = original; + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], copy_assigned[i]); } } TEST(InlinedVectorTest, MoveConstructorInlined) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength - 1; - typedef InlinedVector IntVec8; IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); + FillVector(&original, kInlinedFillSize); IntVec8 tmp(original); auto* old_data = tmp.data(); IntVec8 move_constructed(std::move(tmp)); @@ -210,13 +192,8 @@ TEST(InlinedVectorTest, MoveConstructorInlined) { } TEST(InlinedVectorTest, MoveConstructorAllocated) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength + 1; - typedef InlinedVector IntVec8; IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); + FillVector(&original, kAllocatedFillSize); IntVec8 tmp(original); auto* old_data = tmp.data(); IntVec8 move_constructed(std::move(tmp)); @@ -227,76 +204,64 @@ TEST(InlinedVectorTest, MoveConstructorAllocated) { EXPECT_EQ(move_constructed.data(), old_data); } -TEST(InlinedVectorTest, MoveAssignmentInlined) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength - 1; - typedef InlinedVector IntVec8; +TEST(InlinedVectorTest, MoveAssignmentInlinedInlined) { IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); - // move assigned vector is inlined - { - IntVec8 move_assigned; - FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements - IntVec8 tmp(original); - auto* old_data = tmp.data(); - move_assigned = std::move(tmp); - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], move_assigned[i]); - } - // original data was inlined so it should have been copied, not moved. - EXPECT_NE(move_assigned.data(), old_data); + FillVector(&original, kInlinedFillSize); + IntVec8 move_assigned; + FillVector(&move_assigned, kInlinedFillSize, 99); // Add dummy elements + IntVec8 tmp(original); + auto* old_data = tmp.data(); + move_assigned = std::move(tmp); + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], move_assigned[i]); } - // move assigned vector is allocated - { - IntVec8 move_assigned; - FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements - IntVec8 tmp(original); - auto* old_data = tmp.data(); - move_assigned = std::move(tmp); - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], move_assigned[i]); - } - // original data was inlined so it should have been copied, not moved. - EXPECT_NE(move_assigned.data(), old_data); + // original data was inlined so it should have been copied, not moved. + EXPECT_NE(move_assigned.data(), old_data); +} + +TEST(InlinedVectorTest, MoveAssignmentInlinedAllocated) { + IntVec8 original; + FillVector(&original, kInlinedFillSize); + IntVec8 move_assigned; + FillVector(&move_assigned, kAllocatedFillSize, 99); // Add dummy elements + IntVec8 tmp(original); + auto* old_data = tmp.data(); + move_assigned = std::move(tmp); + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], move_assigned[i]); } + // original data was inlined so it should have been copied, not moved. + EXPECT_NE(move_assigned.data(), old_data); } -TEST(InlinedVectorTest, MoveAssignmentAllocated) { - const size_t kInlinedLength = 8; - const size_t kFillSize = kInlinedLength + 1; - typedef InlinedVector IntVec8; +TEST(InlinedVectorTest, MoveAssignmentAllocatedInlined) { IntVec8 original; - FillVector(&original, kFillSize); - EXPECT_EQ(kFillSize, original.size()); - EXPECT_LE(kFillSize, original.capacity()); - // move assigned vector is inlined - { - IntVec8 move_assigned; - FillVector(&move_assigned, kInlinedLength - 1, 99); // Add dummy elements - IntVec8 tmp(original); - auto* old_data = tmp.data(); - move_assigned = std::move(tmp); - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], move_assigned[i]); - } - // original data was allocated so it should have been moved, not copied. - EXPECT_EQ(move_assigned.data(), old_data); + FillVector(&original, kAllocatedFillSize); + IntVec8 move_assigned; + FillVector(&move_assigned, kInlinedFillSize, 99); // Add dummy elements + IntVec8 tmp(original); + auto* old_data = tmp.data(); + move_assigned = std::move(tmp); + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], move_assigned[i]); } - // move assigned vector is allocated - { - IntVec8 move_assigned; - FillVector(&move_assigned, kInlinedLength + 1, 99); // Add dummy elements - IntVec8 tmp(original); - auto* old_data = tmp.data(); - move_assigned = std::move(tmp); - for (size_t i = 0; i < original.size(); ++i) { - EXPECT_EQ(original[i], move_assigned[i]); - } - // original data was allocated so it should have been moved, not copied. - EXPECT_EQ(move_assigned.data(), old_data); + // original data was allocated so it should have been moved, not copied. + EXPECT_EQ(move_assigned.data(), old_data); +} + +TEST(InlinedVectorTest, MoveAssignmentAllocatedAllocated) { + IntVec8 original; + FillVector(&original, kAllocatedFillSize); + IntVec8 move_assigned; + FillVector(&move_assigned, kAllocatedFillSize, 99); // Add dummy elements + IntVec8 tmp(original); + auto* old_data = tmp.data(); + move_assigned = std::move(tmp); + for (size_t i = 0; i < original.size(); ++i) { + EXPECT_EQ(original[i], move_assigned[i]); } + // original data was allocated so it should have been moved, not copied. + EXPECT_EQ(move_assigned.data(), old_data); } } // namespace testing