Add ListContainer::AppendByMoving

cc/base/list_container_unittest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "cc/base/list_container.h"
 
 #include <vector>
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ skipping 802 matching lines @@
   check_equal();  // One full, one partially full, one empty.
   pop();
   check_equal();  // One full, one empty.
   push();
   pop();
   pop();
   ASSERT_TRUE(list.empty());
   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