Fix non-copyable class's optional move.

BUG=784732
TEST=Ran base_unittests -gtest_filter=*Optional*

Change-Id: Ibb5d7cc5d62deacdba7f811f5a7b83c1c58c3907
Reviewed-on: https://chromium-review.googlesource.com/855976Reviewed-by: danakj <danakj@chromium.org>
Commit-Queue: Hidehiko Abe <hidehiko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#530663}

base/optional.h

@@ -44,6 +44,15 @@ struct OptionalStorageBase {
 
   // When T is not trivially destructible we must call its
   // destructor before deallocating its memory.
+  // Note that this hides the (implicitly declared) move constructor, which
+  // would be used for constexpr move constructor in OptionalStorage<T>.
+  // It is needed iff T is trivially move constructible. However, the current
+  // is_trivially_{copy,move}_constructible implementation requires
+  // is_trivially_destructible (which looks a bug, cf:
+  // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51452 and
+  // http://cplusplus.github.io/LWG/lwg-active.html#2116), so it is not
+  // necessary for this case at the moment. Please see also the destructor
+  // comment in "is_trivially_destructible = true" specialization below.
   ~OptionalStorageBase() {
     if (!is_null_)
       value_.~T();
@@ -77,9 +86,18 @@ struct OptionalStorageBase<T, true /* trivially destructible */> {
       : is_null_(false), value_(std::forward<Args>(args)...) {}
 
   // When T is trivially destructible (i.e. its destructor does nothing) there
-  // is no need to call it. Explicitly defaulting the destructor means it's not
-  // user-provided. Those two together make this destructor trivial.
-  ~OptionalStorageBase() = default;
+  // is no need to call it. Implicitly defined destructor is trivial, because
+  // both members (bool and union containing only variants which are trivially
+  // destructible) are trivially destructible.
+  // Explicitly-defaulted destructor is also trivial, but do not use it here,
+  // because it hides the implicit move constructor. It is needed to implement
+  // constexpr move constructor in OptionalStorage iff T is trivially move
+  // constructible. Note that, if T is trivially move constructible, the move
+  // constructor of OptionalStorageBase<T> is also implicitly defined and it is
+  // trivially move constructor. If T is not trivially move constructible,
+  // "not declaring move constructor without destructor declaration" here means
+  // "delete move constructor", which works because any move constructor of
+  // OptionalStorage will not refer to it in that case.
 
   template <class... Args>
   void Init(Args&&... args) {

base/optional_unittest.cc

@@ -115,6 +115,35 @@ class DeletedDefaultConstructor {
   int foo_;
 };
 
+class DeletedCopyConstructor {
+ public:
+  explicit DeletedCopyConstructor(int foo) : foo_(foo) {}
+  DeletedCopyConstructor(const DeletedCopyConstructor&) = delete;
+  DeletedCopyConstructor(DeletedCopyConstructor&&) = default;
+
+  int foo() const { return foo_; }
+
+ private:
+  int foo_;
+};
+
+class NonTriviallyDestructibleDeletedCopyConstructor {
+ public:
+  explicit NonTriviallyDestructibleDeletedCopyConstructor(int foo)
+      : foo_(foo) {}
+  NonTriviallyDestructibleDeletedCopyConstructor(
+      const NonTriviallyDestructibleDeletedCopyConstructor&) = delete;
+  NonTriviallyDestructibleDeletedCopyConstructor(
+      NonTriviallyDestructibleDeletedCopyConstructor&&) = default;
+
+  ~NonTriviallyDestructibleDeletedCopyConstructor() {}
+
+  int foo() const { return foo_; }
+
+ private:
+  int foo_;
+};
+
 class DeleteNewOperators {
  public:
   void* operator new(size_t) = delete;
@@ -168,6 +197,15 @@ TEST(OptionalTest, CopyConstructor) {
     EXPECT_EQ(first, other);
   }
 
+  {
+    const Optional<std::string> first("foo");
+    Optional<std::string> other(first);
+
+    EXPECT_TRUE(other);
+    EXPECT_EQ(other.value(), "foo");
+    EXPECT_EQ(first, other);
+  }
+
   {
     Optional<TestObject> first(TestObject(3, 0.1));
     Optional<TestObject> other(first);
@@ -210,33 +248,57 @@ TEST(OptionalTest, MoveConstructor) {
     constexpr Optional<float> first(0.1f);
     constexpr Optional<float> second(std::move(first));
 
-    EXPECT_TRUE(second);
+    EXPECT_TRUE(second.has_value());
     EXPECT_EQ(second.value(), 0.1f);
 
-    EXPECT_TRUE(first);
+    EXPECT_TRUE(first.has_value());
   }
 
   {
     Optional<std::string> first("foo");
     Optional<std::string> second(std::move(first));
 
-    EXPECT_TRUE(second);
+    EXPECT_TRUE(second.has_value());
     EXPECT_EQ("foo", second.value());
 
-    EXPECT_TRUE(first);
+    EXPECT_TRUE(first.has_value());
   }
 
   {
     Optional<TestObject> first(TestObject(3, 0.1));
     Optional<TestObject> second(std::move(first));
 
-    EXPECT_TRUE(!!second);
+    EXPECT_TRUE(second.has_value());
     EXPECT_EQ(TestObject::State::MOVE_CONSTRUCTED, second->state());
     EXPECT_TRUE(TestObject(3, 0.1) == second.value());
 
-    EXPECT_TRUE(!!first);
+    EXPECT_TRUE(first.has_value());
     EXPECT_EQ(TestObject::State::MOVED_FROM, first->state());
   }
+
+  // Even if copy constructor is deleted, move constructor needs to work.
+  // Note that it couldn't be constexpr.
+  {
+    Optional<DeletedCopyConstructor> first(in_place, 42);
+    Optional<DeletedCopyConstructor> second(std::move(first));
+
+    EXPECT_TRUE(second.has_value());
+    EXPECT_EQ(42, second->foo());
+
+    EXPECT_TRUE(first.has_value());
+  }
+
+  {
+    Optional<NonTriviallyDestructibleDeletedCopyConstructor> first(in_place,
+                                                                   42);
+    Optional<NonTriviallyDestructibleDeletedCopyConstructor> second(
+        std::move(first));
+
+    EXPECT_TRUE(second.has_value());
+    EXPECT_EQ(42, second->foo());
+
+    EXPECT_TRUE(first.has_value());
+  }
 }
 
 TEST(OptionalTest, MoveValueConstructor) {