From 99d6990ec46494f8cf9971d5eb53429a599baf0f Mon Sep 17 00:00:00 2001 From: Phil Willoughby Date: Wed, 26 Jul 2017 05:52:11 -0700 Subject: [PATCH] Update Optional Summary: Now const-optimizer safe, and safe when the contained value overloads unary operator& Reviewed By: yfeldblum Differential Revision: D5480170 fbshipit-source-id: 3b53b0b6ce608857aa29d3f61eccd0b793b4cddc --- folly/Optional.h | 144 ++++++++++++++++++++++++----------------------- 1 file changed, 74 insertions(+), 70 deletions(-) diff --git a/folly/Optional.h b/folly/Optional.h index 54d9d198..cdf05201 100644 --- a/folly/Optional.h +++ b/folly/Optional.h @@ -53,6 +53,7 @@ * cout << *v << endl; * } */ + #include #include #include @@ -60,6 +61,7 @@ #include #include +#include #include #include @@ -101,14 +103,14 @@ class Optional { Optional(const Optional& src) noexcept( std::is_nothrow_copy_constructible::value) { if (src.hasValue()) { - construct(src.value()); + storage_.construct(src.value()); } } Optional(Optional&& src) noexcept( std::is_nothrow_move_constructible::value) { if (src.hasValue()) { - construct(std::move(src.value())); + storage_.construct(std::move(src.value())); src.clear(); } } @@ -117,18 +119,18 @@ class Optional { /* implicit */ Optional(Value&& newValue) noexcept( std::is_nothrow_move_constructible::value) { - construct(std::move(newValue)); + storage_.construct(std::move(newValue)); } /* implicit */ Optional(const Value& newValue) noexcept( std::is_nothrow_copy_constructible::value) { - construct(newValue); + storage_.construct(newValue); } template explicit Optional(in_place_t, Args&&... args) noexcept( std::is_nothrow_constructible::value) { - construct(std::forward(args)...); + storage_.construct(std::forward(args)...); } void assign(const None&) { @@ -156,17 +158,17 @@ class Optional { void assign(Value&& newValue) { if (hasValue()) { - storage_.value = std::move(newValue); + *storage_.value_pointer() = std::move(newValue); } else { - construct(std::move(newValue)); + storage_.construct(std::move(newValue)); } } void assign(const Value& newValue) { if (hasValue()) { - storage_.value = newValue; + *storage_.value_pointer() = newValue; } else { - construct(newValue); + storage_.construct(newValue); } } @@ -191,7 +193,7 @@ class Optional { template void emplace(Args&&... args) { clear(); - construct(std::forward(args)...); + storage_.construct(std::forward(args)...); } void clear() { @@ -200,34 +202,34 @@ class Optional { const Value& value() const & { require_value(); - return storage_.value; + return *storage_.value_pointer(); } Value& value() & { require_value(); - return storage_.value; + return *storage_.value_pointer(); } Value&& value() && { require_value(); - return std::move(storage_.value); + return std::move(*storage_.value_pointer()); } const Value&& value() const && { require_value(); - return std::move(storage_.value); + return std::move(*storage_.value_pointer()); } const Value* get_pointer() const & { - return storage_.hasValue ? &storage_.value : nullptr; + return storage_.value_pointer(); } Value* get_pointer() & { - return storage_.hasValue ? &storage_.value : nullptr; + return storage_.value_pointer(); } Value* get_pointer() && = delete; bool hasValue() const { - return storage_.hasValue; + return storage_.hasValue(); } explicit operator bool() const { @@ -257,8 +259,8 @@ class Optional { // Return a copy of the value if set, or a given default if not. template Value value_or(U&& dflt) const & { - if (storage_.hasValue) { - return storage_.value; + if (storage_.hasValue()) { + return *storage_.value_pointer(); } return std::forward(dflt); @@ -266,8 +268,8 @@ class Optional { template Value value_or(U&& dflt) && { - if (storage_.hasValue) { - return std::move(storage_.value); + if (storage_.hasValue()) { + return std::move(*storage_.value_pointer()); } return std::forward(dflt); @@ -275,73 +277,75 @@ class Optional { private: void require_value() const { - if (!storage_.hasValue) { + if (!storage_.hasValue()) { detail::throw_optional_empty_exception(); } } - template - void construct(Args&&... args) { - const void* ptr = &storage_.value; - // for supporting const types - new (const_cast(ptr)) Value(std::forward(args)...); - storage_.hasValue = true; - } - struct StorageTriviallyDestructible { - // The union trick allows to initialize the Optional's memory, - // so that compiler/tools don't complain about uninitialized memory, - // without actually calling Value's default constructor. - // The rest of the implementation enforces that hasValue/value are - // synchronized. - union { - bool hasValue; - struct { - bool paddingForHasValue_[1]; - Value value; - }; - }; - - StorageTriviallyDestructible() : hasValue{false} {} + protected: + bool hasValue_; + std::aligned_storage_t value_[1]; + public: + StorageTriviallyDestructible() : hasValue_{false} {} void clear() { - hasValue = false; + hasValue_ = false; } }; struct StorageNonTriviallyDestructible { - // See StorageTriviallyDestructible's union - union { - bool hasValue; - struct { - bool paddingForHasValue_[1]; - Value value; - }; - }; - - FOLLY_PUSH_WARNING - // These are both informational warnings, but they trigger rare enough - // that we've left them enabled. - FOLLY_MSVC_DISABLE_WARNING(4587) // constructor of .value is not called - FOLLY_MSVC_DISABLE_WARNING(4588) // destructor of .value is not called - StorageNonTriviallyDestructible() : hasValue{false} {} + protected: + bool hasValue_; + std::aligned_storage_t value_[1]; + + public: + StorageNonTriviallyDestructible() : hasValue_{false} {} ~StorageNonTriviallyDestructible() { clear(); } - FOLLY_POP_WARNING void clear() { - if (hasValue) { - hasValue = false; - value.~Value(); + if (hasValue_) { + hasValue_ = false; + launder(reinterpret_cast(value_))->~Value(); } } }; - using Storage = typename std::conditional< - std::is_trivially_destructible::value, - StorageTriviallyDestructible, - StorageNonTriviallyDestructible>::type; + struct Storage : std::conditional_t< + std::is_trivially_destructible::value, + StorageTriviallyDestructible, + StorageNonTriviallyDestructible> { + bool hasValue() const { + return this->hasValue_; + } + + Value* value_pointer() { + if (this->hasValue_) { + return launder(reinterpret_cast(this->value_)); + } + return nullptr; + } + + Value const* value_pointer() const { + if (this->hasValue_) { + return launder(reinterpret_cast(this->value_)); + } + return nullptr; + } + + template + void construct(Args&&... args) { + new (raw_pointer()) Value(std::forward(args)...); + this->hasValue_ = true; + } + + private: + void* raw_pointer() { + return static_cast(this->value_); + } + }; Storage storage_; }; @@ -367,7 +371,7 @@ void swap(Optional& a, Optional& b) { } } -template ::type>> +template >> Opt make_optional(T&& v) { return Opt(std::forward(v)); } @@ -467,7 +471,7 @@ struct hash> { if (!obj.hasValue()) { return 0; } - return hash::type>()(*obj); + return hash>()(*obj); } }; FOLLY_NAMESPACE_STD_END -- 2.34.1