folly::Optional<T> should be trivially destructible when T is

Summary:There doesn't appear to be a way to use std::enable_if on a
destructor, so conditionally select from one of two storage types.

Reviewed By: yfeldblum

Differential Revision: D2923902

fb-gh-sync-id: 2def8d1031d70379fd84e8eb555dad9d2b4996f2
shipit-source-id: 2def8d1031d70379fd84e8eb555dad9d2b4996f2

diff --git a/folly/Optional.h b/folly/Optional.h
index 5750ffd8fb74c7c363ddb73e6fbca4f2d00c5a86..2e04e332d0f55f6d15f8710764af0baf4385f9c1 100644 (file)
--- a/folly/Optional.h
+++ b/folly/Optional.h
@@ -94,8 +94,7 @@ class Optional {
   static_assert(!std::is_abstract<Value>::value,
                 "Optional may not be used with abstract types");
 
-  Optional() noexcept
-    : hasValue_(false) {
+  Optional() noexcept {
   }
 
   Optional(const Optional& src)
@@ -103,8 +102,6 @@ class Optional {
 
     if (src.hasValue()) {
       construct(src.value());
-    } else {
-      hasValue_ = false;
     }
   }
 
@@ -114,13 +111,10 @@ class Optional {
     if (src.hasValue()) {
       construct(std::move(src.value()));
       src.clear();
-    } else {
-      hasValue_ = false;
     }
   }
 
-  /* implicit */ Optional(const None&) noexcept
-    : hasValue_(false) {
+  /* implicit */ Optional(const None&) noexcept {
   }
 
   /* implicit */ Optional(Value&& newValue)
@@ -133,10 +127,6 @@ class Optional {
     construct(newValue);
   }
 
-  ~Optional() noexcept {
-    clear();
-  }
-
   void assign(const None&) {
     clear();
   }
@@ -162,7 +152,7 @@ class Optional {
 
   void assign(Value&& newValue) {
     if (hasValue()) {
-      value_ = std::move(newValue);
+      storage_.value = std::move(newValue);
     } else {
       construct(std::move(newValue));
     }
@@ -170,7 +160,7 @@ class Optional {
 
   void assign(const Value& newValue) {
     if (hasValue()) {
-      value_ = newValue;
+      storage_.value = newValue;
     } else {
       construct(newValue);
     }
@@ -203,32 +193,33 @@ class Optional {
   }
 
   void clear() {
-    if (hasValue()) {
-      hasValue_ = false;
-      value_.~Value();
-    }
+    storage_.clear();
   }
 
   const Value& value() const& {
     require_value();
-    return value_;
+    return storage_.value;
   }
 
   Value& value() & {
     require_value();
-    return value_;
+    return storage_.value;
   }
 
   Value value() && {
     require_value();
-    return std::move(value_);
+    return std::move(storage_.value);
   }
 
-  const Value* get_pointer() const&  { return hasValue_ ? &value_ : nullptr; }
-        Value* get_pointer()      &  { return hasValue_ ? &value_ : nullptr; }
-        Value* get_pointer()      && = delete;
+  const Value* get_pointer() const&  {
+    return storage_.hasValue ? &storage_.value : nullptr;
+  }
+  Value* get_pointer() & {
+    return storage_.hasValue ? &storage_.value : nullptr;
+  }
+  Value* get_pointer() && = delete;
 
-  bool hasValue() const { return hasValue_; }
+  bool hasValue() const { return storage_.hasValue; }
 
   explicit operator bool() const {
     return hasValue();
@@ -244,32 +235,74 @@ class Optional {
   // Return a copy of the value if set, or a given default if not.
   template <class U>
   Value value_or(U&& dflt) const& {
-    return hasValue_ ? value_ : std::forward<U>(dflt);
+    if (storage_.hasValue) {
+      return storage_.value;
+    }
+
+    return std::forward<U>(dflt);
   }
 
   template <class U>
   Value value_or(U&& dflt) && {
-    return hasValue_ ? std::move(value_) : std::forward<U>(dflt);
+    if (storage_.hasValue) {
+      return std::move(storage_.value);
+    }
+
+    return std::forward<U>(dflt);
   }
 
  private:
   void require_value() const {
-    if (!hasValue_) {
+    if (!storage_.hasValue) {
       throw OptionalEmptyException();
     }
   }
 
   template<class... Args>
   void construct(Args&&... args) {
-    const void* ptr = &value_;
+    const void* ptr = &storage_.value;
     // for supporting const types
     new(const_cast<void*>(ptr)) Value(std::forward<Args>(args)...);
-    hasValue_ = true;
+    storage_.hasValue = true;
   }
 
-  // uninitialized
-  union { Value value_; };
-  bool hasValue_;
+  struct StorageTriviallyDestructible {
+    // uninitialized
+    union { Value value; };
+    bool hasValue;
+
+    StorageTriviallyDestructible() : hasValue{false} {}
+
+    void clear() {
+      hasValue = false;
+    }
+  };
+
+  struct StorageNonTriviallyDestructible {
+    // uninitialized
+    union { Value value; };
+    bool hasValue;
+
+    StorageNonTriviallyDestructible() : hasValue{false} {}
+
+    ~StorageNonTriviallyDestructible() {
+      clear();
+    }
+
+    void clear() {
+      if (hasValue) {
+        hasValue = false;
+        value.~Value();
+      }
+    }
+  };
+
+  using Storage =
+    typename std::conditional<std::is_trivially_destructible<Value>::value,
+                              StorageTriviallyDestructible,
+                              StorageNonTriviallyDestructible>::type;
+
+  Storage storage_;
 };
 
 #if defined(__GNUC__) && !defined(__clang__)

diff --git a/folly/test/OptionalTest.cpp b/folly/test/OptionalTest.cpp
index 35a262c8cca6fbd05065ff76207554951bbbf459..1082b35bbd3c72c1ef2663cae1adde999cbe45e9 100644 (file)
--- a/folly/test/OptionalTest.cpp
+++ b/folly/test/OptionalTest.cpp
@@ -546,4 +546,17 @@ TEST(Optional, NoThrowDefaultConstructible) {
   EXPECT_TRUE(std::is_nothrow_default_constructible<Optional<bool>>::value);
 }
 
+struct NoDestructor {};
+
+struct WithDestructor {
+  ~WithDestructor();
+};
+
+TEST(Optional, TriviallyDestructible) {
+  // These could all be static_asserts but EXPECT_* give much nicer output on
+  // failure.
+  EXPECT_TRUE(std::is_trivially_destructible<Optional<NoDestructor>>::value);
+  EXPECT_TRUE(std::is_trivially_destructible<Optional<int>>::value);
+  EXPECT_FALSE(std::is_trivially_destructible<Optional<WithDestructor>>::value);
+}
 }