Add ListContainer::AppendByMoving

cc/base/list_container_unittest.cc

Index: cc/base/list_container_unittest.cc
diff --git a/cc/base/list_container_unittest.cc b/cc/base/list_container_unittest.cc
index 021009b5605c2901392cad77bf2eb1cc64209a80..dd2661ce7aea510064a53b60b881c57b2f8ddb9d 100644
--- a/cc/base/list_container_unittest.cc
+++ b/cc/base/list_container_unittest.cc
@@ -820,5 +820,131 @@ TEST(ListContainerTest, RemoveLastIteration) {
   check_equal();  // Empty.
 }
 
+TEST(ListContainerTest, AppendByMovingSameList) {
+  ListContainer<SimpleDerivedElement> list(kCurrentLargestDerivedElementSize);
+  list.AllocateAndConstruct<SimpleDerivedElementConstructMagicNumberOne>();
+
+  list.AppendByMoving(list.front());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list.back()->get_value());
+  EXPECT_EQ(2u, list.size());
+
+  list.front()->set_value(kMagicNumberToUseForSimpleDerivedElementTwo);
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementTwo,
+            list.front()->get_value());
+  list.AppendByMoving(list.front());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementTwo,
+            list.back()->get_value());
+  EXPECT_EQ(3u, list.size());
+}
+
+TEST(ListContainerTest, AppendByMovingDoesNotDestruct) {
+  ListContainer<DerivedElement> list_1(kCurrentLargestDerivedElementSize);
+  ListContainer<DerivedElement> list_2(kCurrentLargestDerivedElementSize);
+  ListContainer<DerivedElement> list_3(kCurrentLargestDerivedElementSize);
+
+  MockDerivedElement* de_1 = list_1.AllocateAndConstruct<MockDerivedElement>();
+  list_2.AppendByMoving(de_1);
+  list_3.AppendByMoving(list_2.back());
+  list_3.AppendByMoving(list_3.back());
+

[danakj, 2015/06/23 20:01:12]

Can you VerifyExpectations here that it wasn't destructed yet?

[pdr., 2015/06/23 21:42:42]

Done.

+  EXPECT_CALL(*de_1, Destruct());

[danakj, 2015/06/23 20:01:12]

Then do this as the last step in the test?

[pdr., 2015/06/23 21:42:42]

Done (Now the last step in a sequence.)

+  EXPECT_EQ(1u, list_1.size());
+  EXPECT_EQ(1u, list_2.size());
+  EXPECT_EQ(2u, list_3.size());
+}
+
+TEST(ListContainerTest, AppendByMovingReplacesSourceWithNewDerivedElement) {
+  ListContainer<SimpleDerivedElementConstructMagicNumberOne> list_1(
+      kCurrentLargestDerivedElementSize);
+  ListContainer<SimpleDerivedElementConstructMagicNumberTwo> list_2(
+      kCurrentLargestDerivedElementSize);
+
+  list_1.AllocateAndConstruct<SimpleDerivedElementConstructMagicNumberOne>();
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list_1.front()->get_value());
+
+  list_2.AppendByMoving(list_1.front());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list_1.front()->get_value());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list_2.front()->get_value());
+
+  list_1.AppendByMoving(list_2.back());

[danakj, 2015/06/23 20:01:13]

Can you change the value of the item before moving it for one of these cases, so
we can see that the old element is being replaced, not just staying the same?
(And a comment explaining what you're doing pls)

[pdr., 2015/06/23 21:42:42]

Done.

I needed to add a new constant, kMagicNumberToUseForSimpleDerivedElementThree,
as using One or Two could hide bugs.

+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list_1.front()->get_value());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list_1.back()->get_value());
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementTwo,
+            list_2.back()->get_value());
+
+  EXPECT_EQ(2u, list_1.size());

[danakj, 2015/06/23 20:01:13]

Can you leave a comment explaining these numbers?

[pdr., 2015/06/23 21:42:42]

Done. The comment now reads:
// AppendByMoving replaces the source element with a new derived element so
// we do not expect sizes to shrink after AppendByMoving is called.
EXPECT_EQ(2u, list_1.size()); // One direct allocation and one AppendByMoving.
EXPECT_EQ(1u, list_2.size()); // One AppendByMoving.

+  EXPECT_EQ(1u, list_2.size());
+}
+
+const size_t kLongCountForLongSimpleDerivedElement = 5;
+class LongSimpleDerivedElement : public SimpleDerivedElement {

[danakj, 2015/06/23 20:01:12]

whitespace before the class

[pdr., 2015/06/23 21:42:42]

Done. The new whitespace looks like:

+ public:
+  ~LongSimpleDerivedElement() override {}
+  void set_all_values(unsigned long value) {

[danakj, 2015/06/23 20:01:12]

SetAllValues

[pdr., 2015/06/23 21:42:42]

Done.

+    for (size_t i = 0; i < kLongCountForLongSimpleDerivedElement; i++)
+      values[i] = value;
+  }
+  bool all_values_equal_to(size_t value) const {

[danakj, 2015/06/23 20:01:12]

AreAllValuesEqualTo

[pdr., 2015/06/23 21:42:42]

Done.

+    for (size_t i = 1; i < kLongCountForLongSimpleDerivedElement; i++) {
+      if (values[i] != values[0])
+        return false;
+    }
+    return true;
+  }
+
+ private:
+  unsigned long values[kLongCountForLongSimpleDerivedElement];
+};
+
+const unsigned long kMagicNumberToUseForLongSimpleDerivedElement = -1618ul;

[danakj, 2015/06/23 20:01:44]

e:\b\build\slave\win\build\src\cc\base\list_container_unittest.cc(905) : warning
C4146: unary minus operator applied to unsigned type, result still unsigned


Use a positive value here?

[pdr., 2015/06/23 21:42:42]

Done.

I was hoping to use a value that would be sensitive to begin stomped (mostly
1's) on but that's probably overkill.

+class LongSimpleDerivedElementConstructMagicNumber

[danakj, 2015/06/23 20:01:12]

whitespace before the class pls

[pdr., 2015/06/23 21:42:42]

Done.

+    : public LongSimpleDerivedElement {
+ public:
+  LongSimpleDerivedElementConstructMagicNumber() : LongSimpleDerivedElement() {

[danakj, 2015/06/23 20:01:13]

you don't need to explicitly call default constructor right?

[pdr., 2015/06/23 21:42:42]

Good catch. Fixed.

I also updated SimpleDerivedElementConstructMagicNumberOne and
SimpleDerivedElementConstructMagicNumberTwo which had this mistake as well.

+    set_all_values(kMagicNumberToUseForLongSimpleDerivedElement);
+  }
+};
+
+TEST(ListContainerTest, AppendByMovingLongAndSimpleDerivedElements) {
+  static_assert(sizeof(LongSimpleDerivedElement) > sizeof(SimpleDerivedElement),
+                "LongSimpleDerivedElement should be larger than "
+                "SimpleDerivedElement's size.");
+  static_assert(sizeof(LongSimpleDerivedElement) <= kLargestDerivedElementSize,
+                "LongSimpleDerivedElement should be smaller than the maximum "
+                "DerivedElement size.");
+
+  ListContainer<SimpleDerivedElement> list(kCurrentLargestDerivedElementSize);
+  list.AllocateAndConstruct<LongSimpleDerivedElementConstructMagicNumber>();
+  list.AllocateAndConstruct<SimpleDerivedElementConstructMagicNumberOne>();
+
+  EXPECT_TRUE(
+      static_cast<LongSimpleDerivedElement*>(list.front())
+          ->all_values_equal_to(kMagicNumberToUseForLongSimpleDerivedElement));
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list.back()->get_value());
+
+  SimpleDerivedElement* simple_element = list.back();
+  list.AppendByMoving(list.front());

[danakj, 2015/06/23 20:01:12]

Can you comment explaining what you are testing here? (The fact that the type
passed is a base class which is smaller than what is actually being copied)

[pdr., 2015/06/23 21:42:42]

Done. The comment now reads:
// Test that moving a simple derived element actually moves enough data so
// that the LongSimpleDerivedElement at this location is entirely moved.

+  EXPECT_TRUE(
+      static_cast<LongSimpleDerivedElement*>(list.back())
+          ->all_values_equal_to(kMagicNumberToUseForLongSimpleDerivedElement));
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            simple_element->get_value());
+
+  LongSimpleDerivedElement* long_element =
+      static_cast<LongSimpleDerivedElement*>(list.back());
+  list.AppendByMoving(simple_element);
+  EXPECT_TRUE(long_element->all_values_equal_to(
+      kMagicNumberToUseForLongSimpleDerivedElement));
+  EXPECT_EQ(kMagicNumberToUseForSimpleDerivedElementOne,
+            list.back()->get_value());
+}
+
 }  // namespace
 }  // namespace cc