From 8fa647baa1e466bfbb5a8295bd6fc4a9fb5eb43c Mon Sep 17 00:00:00 2001 From: Hidehiko Abe Date: Fri, 19 Jan 2018 23:50:24 +0000 Subject: [PATCH 07/10] 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/855976 Reviewed-by: danakj Commit-Queue: Hidehiko Abe Cr-Commit-Position: refs/heads/master@{#530663} --- base/optional.h | 24 +++++++++++++-- base/optional_unittest.cc | 74 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 89 insertions(+), 9 deletions(-) diff --git a/base/optional.h b/base/optional.h index c763acf824ee..5a50eb455be6 100644 --- a/base/optional.h +++ b/base/optional.h @@ -45,6 +45,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. + // 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(); @@ -78,9 +87,18 @@ struct OptionalStorageBase { : is_null_(false), value_(std::forward(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 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 void Init(Args&&... args) { diff --git a/base/optional_unittest.cc b/base/optional_unittest.cc index 91e63e75d0db..7cc05ef2987d 100644 --- a/base/optional_unittest.cc +++ b/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 first("foo"); + Optional other(first); + + EXPECT_TRUE(other); + EXPECT_EQ(other.value(), "foo"); + EXPECT_EQ(first, other); + } + { Optional first(TestObject(3, 0.1)); Optional other(first); @@ -210,33 +248,57 @@ TEST(OptionalTest, MoveConstructor) { constexpr Optional first(0.1f); constexpr Optional 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 first("foo"); Optional 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 first(TestObject(3, 0.1)); Optional 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 first(in_place, 42); + Optional second(std::move(first)); + + EXPECT_TRUE(second.has_value()); + EXPECT_EQ(42, second->foo()); + + EXPECT_TRUE(first.has_value()); + } + + { + Optional first(in_place, + 42); + Optional second( + std::move(first)); + + EXPECT_TRUE(second.has_value()); + EXPECT_EQ(42, second->foo()); + + EXPECT_TRUE(first.has_value()); + } } TEST(OptionalTest, MoveValueConstructor) { -- 2.16.2