Make base::Optional trivially destructible when possible.

base/optional_unittest.cc

Index: base/optional_unittest.cc
diff --git a/base/optional_unittest.cc b/base/optional_unittest.cc
index 812acf2d2207ae17646061e371ee7b970262e24c..72f17feb545b4c47ba918503954f3efba817bfd5 100644
--- a/base/optional_unittest.cc
+++ b/base/optional_unittest.cc
@@ -22,6 +22,9 @@ class TestObject {
     COPY_CONSTRUCTED,
     MOVE_CONSTRUCTED,
     MOVED_FROM,
+    COPY_ASSIGNED,
+    MOVE_ASSIGNED,
+    SWAPPED,
   };
 
   TestObject() : foo_(0), bar_(0.0), state_(State::DEFAULT_CONSTRUCTED) {}
@@ -42,10 +45,26 @@ class TestObject {
   TestObject& operator=(const TestObject& other) {
     foo_ = other.foo_;
     bar_ = other.bar_;
-    state_ = State::COPY_CONSTRUCTED;
+    state_ = State::COPY_ASSIGNED;
     return *this;
   }
 
+  TestObject& operator=(TestObject&& other) {
+    foo_ = other.foo_;
+    bar_ = other.bar_;
+    state_ = State::MOVE_ASSIGNED;
+    other.state_ = State::MOVED_FROM;
+    return *this;
+  }
+
+  void Swap(TestObject* other) {
+    using std::swap;
+    swap(foo_, other->foo_);
+    swap(bar_, other->bar_);
+    state_ = State::SWAPPED;
+    other->state_ = State::SWAPPED;
+  }
+
   bool operator==(const TestObject& other) const {
     return foo_ == other.foo_ && bar_ == other.bar_;
   }
@@ -59,8 +78,24 @@ class TestObject {
   State state_;
 };
 
+// Implementing Swappable concept.
+void swap(TestObject& lhs, TestObject& rhs) {
+  lhs.Swap(&rhs);
+}
+
+class NonTriviallyDestructible {
+  ~NonTriviallyDestructible() {}
+};
+
 }  // anonymous namespace
 
+static_assert(is_trivially_destructible<Optional<int>>::value,
+              "OptionalIsTriviallyDestructible");
+
+static_assert(
+    !is_trivially_destructible<Optional<NonTriviallyDestructible>>::value,
+    "OptionalIsTriviallyDestructible");
+
 TEST(OptionalTest, DefaultConstructor) {
   {
     Optional<float> o;
@@ -215,6 +250,11 @@ TEST(OptionalTest, ConstructorForwardArguments) {
   }
 }
 
+TEST(OptionalTest, NulloptConstructor) {
+  Optional<int> a = base::nullopt;
+  EXPECT_FALSE(a);
+}
+
 TEST(OptionalTest, AssignValue) {
   {
     Optional<float> a;
@@ -245,6 +285,16 @@ TEST(OptionalTest, AssignValue) {
     Optional<TestObject> b(TestObject(3, 0.1));
     EXPECT_TRUE(a == b);
   }
+
+  {
+    Optional<TestObject> a = TestObject(4, 1.0);
+    EXPECT_TRUE(a);
+    a = TestObject(3, 0.1);
+    EXPECT_TRUE(a);
+
+    Optional<TestObject> b(TestObject(3, 0.1));
+    EXPECT_TRUE(a == b);
+  }
 }
 
 TEST(OptionalTest, AssignObject) {
@@ -277,6 +327,16 @@ TEST(OptionalTest, AssignObject) {
     EXPECT_TRUE(a.value() == TestObject(3, 0.1));
     EXPECT_TRUE(a == b);
   }
+
+  {
+    Optional<TestObject> a(TestObject(4, 1.0));
+    Optional<TestObject> b(TestObject(3, 0.1));
+    a = b;
+
+    EXPECT_TRUE(!!a);
+    EXPECT_TRUE(a.value() == TestObject(3, 0.1));
+    EXPECT_TRUE(a == b);
+  }
 }
 
 TEST(OptionalTest, AssignObject_rvalue) {
@@ -312,6 +372,19 @@ TEST(OptionalTest, AssignObject_rvalue) {
     EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED, a->state());

[danakj, 2016/05/25 20:02:08]

I'm a little concerned that this is now testing compiler optimizations, because
it could construct a and then assign to it, but it's optimizing to construct a
from the move(b) directly I guess?

Does this patch pass on debug, release, and official builds? If you can't try
official can you try -Os -O2 and -O0?

I think I'd be more comfortable with just using a MOVED_TO state for both this
and below.

[alshabalin, 2016/05/26 09:18:45]

Why is it an optimization? We default construct |a| which means TestObject is
not constructed at all. Then we call move assignment operator on |a| which calls
InitOrAssign then Init then placement new with move constructor on TestObject.
So MOVE_CONSTRUCTED must be achieved on any path, optimized or not.

Checked Debug, Release, tried with -Os, -O2 and -O0, all on Mac. Tests pass.

If you still want to, I can combine states COPY_CONSTRUCTED and COPY_ASSIGNED
into COPIED_TO, MOVE_CONSTRUCTED and MOVE_ASSIGNED into MOVED_TO. But I think
these extra states are useful in showing that we do really move/copy construct
an underlying object instead of default constructing and then copy/move
assigning to it.

[danakj, 2016/05/26 22:59:27]

Ah you're right, I was thinking of the Optional |a| but it's the TestObject
inside. Thanks.

     EXPECT_EQ(TestObject::State::MOVED_FROM, b->state());
   }
+
+  {
+    Optional<TestObject> a(TestObject(4, 1.0));
+    Optional<TestObject> b(TestObject(3, 0.1));
+    a = std::move(b);
+
+    EXPECT_TRUE(!!a);
+    EXPECT_TRUE(!!b);
+    EXPECT_TRUE(TestObject(3, 0.1) == a.value());
+
+    EXPECT_EQ(TestObject::State::MOVE_ASSIGNED, a->state());
+    EXPECT_EQ(TestObject::State::MOVED_FROM, b->state());
+  }
 }
 
 TEST(OptionalTest, AssignNull) {
@@ -452,6 +525,8 @@ TEST(OptionalTest, Swap_bothValue) {
   EXPECT_TRUE(!!b);
   EXPECT_TRUE(TestObject(1, 0.3) == a.value_or(TestObject(42, 0.42)));
   EXPECT_TRUE(TestObject(0, 0.1) == b.value_or(TestObject(42, 0.42)));
+  EXPECT_EQ(TestObject::State::SWAPPED, a->state());
+  EXPECT_EQ(TestObject::State::SWAPPED, b->state());
 }
 
 TEST(OptionalTest, Emplace) {
@@ -1136,7 +1211,7 @@ TEST(OptionalTest, MakeOptional) {
     EXPECT_TRUE(!!o);
     EXPECT_TRUE(TestObject(0, 0.42) == *o);
     EXPECT_EQ(TestObject::State::MOVED_FROM, value.state());
-    EXPECT_EQ(TestObject::State::COPY_CONSTRUCTED, o->state());
+    EXPECT_EQ(TestObject::State::MOVE_ASSIGNED, o->state());
 
     EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED,
               base::make_optional(std::move(value))->state());
@@ -1184,6 +1259,8 @@ TEST(OptionalTest, NonMemberSwap_bothValue) {
   EXPECT_TRUE(!!b);
   EXPECT_TRUE(TestObject(1, 0.3) == a.value_or(TestObject(42, 0.42)));
   EXPECT_TRUE(TestObject(0, 0.1) == b.value_or(TestObject(42, 0.42)));
+  EXPECT_EQ(TestObject::State::SWAPPED, a->state());
+  EXPECT_EQ(TestObject::State::SWAPPED, b->state());
 }
 
 TEST(OptionalTest, Hash_OptionalReflectsInternal) {