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..988767cfe32cf4916bf5aa3272ebd271f1dc73c1 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,36 @@ 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 {
+  // When T is not trivially destructible we must call its
+  // destructor before deallocating its memory.
+  ~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> {
+  // When T is trivially destructible (i.e. its destructor does nothing)
+  // there is no need to call it.
+  // Since |base::AlignedMemory| is just an array its destructor
+  // is trivial. Explicitly defaulting the destructor means it's not
+  // user-provided. All of this together make this destructor trivial.
+  ~OptionalStorage() = default;
+
+  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 +77,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 +101,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 +109,7 @@ class Optional {
   }
 
   Optional& operator=(const Optional& other) {
-    if (other.is_null_) {
+    if (other.storage_.is_null_) {
       FreeIfNeeded();
       return *this;
     }
@@ -89,7 +119,7 @@ class Optional {
   }
 
   Optional& operator=(Optional&& other) {
-    if (other.is_null_) {
+    if (other.storage_.is_null_) {
       FreeIfNeeded();
       return *this;
     }
@@ -108,14 +138,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 +161,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 +196,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 +207,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 +239,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>