From f688293393f1e481d1aef3b10cd35bb37c1ea7f8 Mon Sep 17 00:00:00 2001 From: Serge Guelton Date: Wed, 13 Feb 2019 18:12:04 +0000 Subject: [PATCH] Re-commit rL353927, patch included Make llvm::Optional trivially copyable when T is trivially copyable This is an ever-recurring issue (see https://bugs.llvm.org/show_bug.cgi?id=39427 and https://bugs.llvm.org/show_bug.cgi?id=35978) but I believe that thanks to https://reviews.llvm.org/D54472 we can now ship a decent implementation of this. Basically the fact that llvm::is_trivially_copyable has a consistent behavior across compilers should prevent any ABI issue, and using in-place new instead of memcpy should keep compiler bugs away. This patch is slightly different from the original revision https://reviews.llvm.org/rL353927 but achieves the same goal. It just avoids going through std::conditional which may the code more explicit. llvm-svn: 353962 --- llvm/include/llvm/ADT/Optional.h | 93 ++++++++++++++++++++--------- llvm/unittests/ADT/OptionalTest.cpp | 20 +++++++ 2 files changed, 85 insertions(+), 28 deletions(-) diff --git a/llvm/include/llvm/ADT/Optional.h b/llvm/include/llvm/ADT/Optional.h index 25a3185064f4..d7b9d5cddc25 100644 --- a/llvm/include/llvm/ADT/Optional.h +++ b/llvm/include/llvm/ADT/Optional.h @@ -29,33 +29,61 @@ namespace llvm { class raw_ostream; namespace optional_detail { -/// Storage for any type. -template ::value> struct OptionalStorage { +template struct OptionalStorageBase { AlignedCharArrayUnion storage; bool hasVal = false; + OptionalStorageBase() = default; + OptionalStorageBase(const T &y) : hasVal(true) { new (storage.buffer) T(y); } + OptionalStorageBase(T &&y) : hasVal(true) { + new (storage.buffer) T(std::move(y)); + } + + T *getPointer() { + assert(hasVal); + return reinterpret_cast(storage.buffer); + } + const T *getPointer() const { + assert(hasVal); + return reinterpret_cast(storage.buffer); + } + OptionalStorageBase &operator=(T &&y) { + hasVal = true; + new (this->storage.buffer) T(std::move(y)); + return *this; + } + OptionalStorageBase &operator=(const T &y) { + hasVal = true; + new (this->storage.buffer) T(y); + return *this; + } + void reset() { this->hasVal = false; } +}; + +/// Storage for any type. +template ::value> +struct OptionalStorage : OptionalStorageBase { OptionalStorage() = default; - OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y); } - OptionalStorage(const OptionalStorage &O) : hasVal(O.hasVal) { - if (hasVal) - new (storage.buffer) T(*O.getPointer()); + OptionalStorage(const T &y) : OptionalStorageBase(y) {} + OptionalStorage(const OptionalStorage &O) : OptionalStorageBase() { + this->hasVal = O.hasVal; + if (this->hasVal) + new (this->storage.buffer) T(*O.getPointer()); } - OptionalStorage(T &&y) : hasVal(true) { - new (storage.buffer) T(std::forward(y)); - } - OptionalStorage(OptionalStorage &&O) : hasVal(O.hasVal) { + OptionalStorage(T &&y) : OptionalStorageBase(std::move(y)) {} + OptionalStorage(OptionalStorage &&O) : OptionalStorageBase() { + this->hasVal = O.hasVal; if (O.hasVal) { - new (storage.buffer) T(std::move(*O.getPointer())); + new (this->storage.buffer) T(std::move(*O.getPointer())); } } OptionalStorage &operator=(T &&y) { - if (hasVal) - *getPointer() = std::move(y); + if (this->hasVal) + *this->getPointer() = std::move(y); else { - new (storage.buffer) T(std::move(y)); - hasVal = true; + OptionalStorageBase::operator=(std::move(y)); } return *this; } @@ -74,11 +102,10 @@ template ::value> struct OptionalSto // requirements (notably: the existence of a default ctor) when implemented // in that way. Careful SFINAE to avoid such pitfalls would be required. OptionalStorage &operator=(const T &y) { - if (hasVal) - *getPointer() = y; + if (this->hasVal) + *this->getPointer() = y; else { - new (storage.buffer) T(y); - hasVal = true; + OptionalStorageBase::operator=(y); } return *this; } @@ -93,20 +120,30 @@ template ::value> struct OptionalSto ~OptionalStorage() { reset(); } void reset() { - if (hasVal) { - (*getPointer()).~T(); - hasVal = false; + if (this->hasVal) { + (*this->getPointer()).~T(); } + OptionalStorageBase::reset(); } +}; - T *getPointer() { - assert(hasVal); - return reinterpret_cast(storage.buffer); +template struct OptionalStorage : OptionalStorageBase { + OptionalStorage() = default; + OptionalStorage(const T &y) : OptionalStorageBase(y) {} + OptionalStorage(const OptionalStorage &O) = default; + OptionalStorage(T &&y) : OptionalStorageBase(std::move(y)) {} + OptionalStorage(OptionalStorage &&O) = default; + OptionalStorage &operator=(T &&y) { + OptionalStorageBase::operator=(std::move(y)); + return *this; } - const T *getPointer() const { - assert(hasVal); - return reinterpret_cast(storage.buffer); + OptionalStorage &operator=(OptionalStorage &&O) = default; + OptionalStorage &operator=(const T &y) { + OptionalStorageBase::operator=(y); + return *this; } + OptionalStorage &operator=(const OptionalStorage &O) = default; + ~OptionalStorage() = default; }; } // namespace optional_detail diff --git a/llvm/unittests/ADT/OptionalTest.cpp b/llvm/unittests/ADT/OptionalTest.cpp index 98adaccca961..c39b6727cd54 100644 --- a/llvm/unittests/ADT/OptionalTest.cpp +++ b/llvm/unittests/ADT/OptionalTest.cpp @@ -18,6 +18,12 @@ using namespace llvm; namespace { +static_assert(llvm::is_trivially_copyable>::value, + "trivially copyable"); + +static_assert(llvm::is_trivially_copyable>>::value, + "trivially copyable"); + struct NonDefaultConstructible { static unsigned CopyConstructions; static unsigned Destructions; @@ -45,6 +51,10 @@ unsigned NonDefaultConstructible::CopyConstructions = 0; unsigned NonDefaultConstructible::Destructions = 0; unsigned NonDefaultConstructible::CopyAssignments = 0; +static_assert( + !llvm::is_trivially_copyable>::value, + "not trivially copyable"); + // Test fixture class OptionalTest : public testing::Test { }; @@ -203,6 +213,10 @@ struct MultiArgConstructor { }; unsigned MultiArgConstructor::Destructions = 0; +static_assert( + !llvm::is_trivially_copyable>::value, + "not trivially copyable"); + TEST_F(OptionalTest, Emplace) { MultiArgConstructor::ResetCounts(); Optional A; @@ -250,6 +264,9 @@ unsigned MoveOnly::MoveConstructions = 0; unsigned MoveOnly::Destructions = 0; unsigned MoveOnly::MoveAssignments = 0; +static_assert(!llvm::is_trivially_copyable>::value, + "not trivially copyable"); + TEST_F(OptionalTest, MoveOnlyNull) { MoveOnly::ResetCounts(); Optional O; @@ -351,6 +368,9 @@ private: unsigned Immovable::Constructions = 0; unsigned Immovable::Destructions = 0; +static_assert(!llvm::is_trivially_copyable>::value, + "not trivially copyable"); + TEST_F(OptionalTest, ImmovableEmplace) { Optional A; Immovable::ResetCounts();