Make base::Optional trivially destructible when possible.

base/optional.h

Index: base/optional.h
diff --git a/base/optional.h b/base/optional.h
index 5ae427f8a9cc2d79b65665fb6f103b15b2230295..1a97795465b5548cbf66f8fbd1557418c6c25e15 100644
--- a/base/optional.h
+++ b/base/optional.h
@@ -9,6 +9,7 @@
 
 #include "base/logging.h"
 #include "base/memory/aligned_memory.h"
+#include "base/template_util.h"
 
 namespace base {
 
@@ -30,6 +31,29 @@ constexpr in_place_t in_place = {};
 // http://en.cppreference.com/w/cpp/utility/optional/nullopt
 constexpr nullopt_t nullopt(0);
 
+namespace internal {
+
+template <typename T, bool = base::is_trivially_destructible<T>::value>
+struct OptionalStorage {
+  ~OptionalStorage() {
+    if (!is_null_)
+      buffer_.template data_as<T>()->~T();
+  }
+
+  bool is_null_ = true;
+  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+};
+
+template <typename T>
+struct OptionalStorage<T, true> {
+  ~OptionalStorage() = default;

[danakj, 2016/05/25 20:02:08]

Can you leave a comment here explaining that this is not calling the destructor
on T and why.

[alshabalin, 2016/05/26 09:18:45]

Done.

+
+  bool is_null_ = true;
+  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+};
+
+}  // namespace internal
+
 // base::Optional is a Chromium version of the C++17 optional class:
 // std::optional documentation:
 // http://en.cppreference.com/w/cpp/utility/optional
@@ -46,16 +70,18 @@ constexpr nullopt_t nullopt(0);
 template <typename T>
 class Optional {
  public:
+  using value_type = T;
+
   constexpr Optional() = default;
   Optional(base::nullopt_t) : Optional() {}
 
   Optional(const Optional& other) {
-    if (!other.is_null_)
+    if (!other.storage_.is_null_)
       Init(other.value());
   }
 
   Optional(Optional&& other) {
-    if (!other.is_null_)
+    if (!other.storage_.is_null_)
       Init(std::move(other.value()));
   }
 
@@ -68,10 +94,7 @@ class Optional {
     emplace(std::forward<Args>(args)...);
   }
 
-  ~Optional() {
-    // TODO(mlamouri): use is_trivially_destructible<T>::value when possible.
-    FreeIfNeeded();
-  }
+  ~Optional() = default;
 
   Optional& operator=(base::nullopt_t) {
     FreeIfNeeded();
@@ -79,7 +102,7 @@ class Optional {
   }
 
   Optional& operator=(const Optional& other) {
-    if (other.is_null_) {
+    if (other.storage_.is_null_) {
       FreeIfNeeded();
       return *this;
     }
@@ -89,7 +112,7 @@ class Optional {
   }
 
   Optional& operator=(Optional&& other) {
-    if (other.is_null_) {
+    if (other.storage_.is_null_) {
       FreeIfNeeded();
       return *this;
     }
@@ -108,14 +131,14 @@ class Optional {
 
   // TODO(mlamouri): can't use 'constexpr' with DCHECK.
   const T* operator->() const {
-    DCHECK(!is_null_);
+    DCHECK(!storage_.is_null_);
     return &value();
   }
 
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T* operator->() {
-    DCHECK(!is_null_);
+    DCHECK(!storage_.is_null_);
     return &value();
   }
 
@@ -131,32 +154,32 @@ class Optional {
   // meant to be 'constexpr const'.
   T&& operator*() && { return std::move(value()); }
 
-  constexpr explicit operator bool() const { return !is_null_; }
+  constexpr explicit operator bool() const { return !storage_.is_null_; }
 
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T& value() & {
-    DCHECK(!is_null_);
-    return *buffer_.template data_as<T>();
+    DCHECK(!storage_.is_null_);
+    return *storage_.buffer_.template data_as<T>();
   }
 
   // TODO(mlamouri): can't use 'constexpr' with DCHECK.
   const T& value() const& {
-    DCHECK(!is_null_);
-    return *buffer_.template data_as<T>();
+    DCHECK(!storage_.is_null_);
+    return *storage_.buffer_.template data_as<T>();
   }
 
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T&& value() && {
-    DCHECK(!is_null_);
-    return std::move(*buffer_.template data_as<T>());
+    DCHECK(!storage_.is_null_);
+    return std::move(*storage_.buffer_.template data_as<T>());
   }
 
   // TODO(mlamouri): can't use 'constexpr' with DCHECK.
   const T&& value() const&& {
-    DCHECK(!is_null_);
-    return std::move(*buffer_.template data_as<T>());
+    DCHECK(!storage_.is_null_);
+    return std::move(*storage_.buffer_.template data_as<T>());
   }
 
   template <class U>
@@ -166,7 +189,8 @@ class Optional {
     //               "T must be copy constructible");
     static_assert(std::is_convertible<U, T>::value,
                   "U must be convertible to T");
-    return is_null_ ? static_cast<T>(std::forward<U>(default_value)) : value();
+    return storage_.is_null_ ? static_cast<T>(std::forward<U>(default_value))
+                             : value();
   }
 
   template <class U>
@@ -176,26 +200,26 @@ class Optional {
     //               "T must be move constructible");
     static_assert(std::is_convertible<U, T>::value,
                   "U must be convertible to T");
-    return is_null_ ? static_cast<T>(std::forward<U>(default_value))
-                    : std::move(value());
+    return storage_.is_null_ ? static_cast<T>(std::forward<U>(default_value))
+                             : std::move(value());
   }
 
   void swap(Optional& other) {
-    if (is_null_ && other.is_null_)
+    if (storage_.is_null_ && other.storage_.is_null_)
       return;
 
-    if (is_null_ != other.is_null_) {
-      if (is_null_) {
-        Init(std::move(*other.buffer_.template data_as<T>()));
+    if (storage_.is_null_ != other.storage_.is_null_) {
+      if (storage_.is_null_) {
+        Init(std::move(*other.storage_.buffer_.template data_as<T>()));
         other.FreeIfNeeded();
       } else {
-        other.Init(std::move(*buffer_.template data_as<T>()));
+        other.Init(std::move(*storage_.buffer_.template data_as<T>()));
         FreeIfNeeded();
       }
       return;
     }
 
-    DCHECK(!is_null_ && !other.is_null_);
+    DCHECK(!storage_.is_null_ && !other.storage_.is_null_);
     using std::swap;
     swap(**this, *other);
   }
@@ -208,47 +232,46 @@ class Optional {
 
  private:
   void Init(const T& value) {
-    DCHECK(is_null_);
-    new (buffer_.template data_as<T>()) T(value);
-    is_null_ = false;
+    DCHECK(storage_.is_null_);
+    new (storage_.buffer_.template data_as<T>()) T(value);
+    storage_.is_null_ = false;
   }
 
   void Init(T&& value) {
-    DCHECK(is_null_);
-    new (buffer_.template data_as<T>()) T(std::move(value));
-    is_null_ = false;
+    DCHECK(storage_.is_null_);
+    new (storage_.buffer_.template data_as<T>()) T(std::move(value));
+    storage_.is_null_ = false;
   }
 
   template <class... Args>
   void Init(Args&&... args) {
-    DCHECK(is_null_);
-    new (buffer_.template data_as<T>()) T(std::forward<Args>(args)...);
-    is_null_ = false;
+    DCHECK(storage_.is_null_);
+    new (storage_.buffer_.template data_as<T>()) T(std::forward<Args>(args)...);
+    storage_.is_null_ = false;
   }
 
   void InitOrAssign(const T& value) {
-    if (is_null_)
+    if (storage_.is_null_)
       Init(value);
     else
-      *buffer_.template data_as<T>() = value;
+      *storage_.buffer_.template data_as<T>() = value;
   }
 
   void InitOrAssign(T&& value) {
-    if (is_null_)
+    if (storage_.is_null_)
       Init(std::move(value));
     else
-      *buffer_.template data_as<T>() = std::move(value);
+      *storage_.buffer_.template data_as<T>() = std::move(value);
   }
 
   void FreeIfNeeded() {
-    if (is_null_)
+    if (storage_.is_null_)
       return;
-    buffer_.template data_as<T>()->~T();
-    is_null_ = true;
+    storage_.buffer_.template data_as<T>()->~T();
+    storage_.is_null_ = true;
   }
 
-  bool is_null_ = true;
-  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+  internal::OptionalStorage<T> storage_;
 };
 
 template <class T>