base::Optional: Use anonymous union instead of base::AlignedMemory

base/optional.h

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef BASE_OPTIONAL_H_
 #define BASE_OPTIONAL_H_
 
 #include <type_traits>
 
 #include "base/logging.h"
-#include "base/memory/aligned_memory.h"
 #include "base/template_util.h"
 
 namespace base {
 
 // Specification:
 // http://en.cppreference.com/w/cpp/utility/optional/in_place_t
 struct in_place_t {};
 
 // Specification:
 // http://en.cppreference.com/w/cpp/utility/optional/nullopt_t
 struct nullopt_t {
   constexpr explicit nullopt_t(int) {}
 };
 
 // Specification:
 // http://en.cppreference.com/w/cpp/utility/optional/in_place
 constexpr in_place_t in_place = {};
 
 // Specification:
 // 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() {};
   // 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();
+      value_.~T();
   }
 
   bool is_null_ = true;
-  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+  union {
+    // empty_ exists so that the union will always be initialized, even when it

[danakj, 2016/08/15 21:22:57]

nit: || around variable names. "|empty_| exists.."

[kwiberg-chromium, 2016/08/16 08:23:47]

Done.

+    // doesn't contain a value. Not initializing it has been observed to
+    // trigger comiler warnings.
+    char empty_ = '\0';
+    T value_;
+  };
 };
 
 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() {};

[danakj, 2016/08/15 21:22:57]

no ; here

also or how about =default; instead?

[kwiberg-chromium, 2016/08/16 08:23:47]

Done.
Doesn't work (and omitting it entirely also doesn't work, for the same reason).
http://en.cppreference.com/w/cpp/language/union says: "If a union contains a
non-static data member with a non-trivial special member function (default
constructor, copy/move constructor, copy/move assignment, or destructor), that
function is deleted by default in the union and needs to be defined explicitly
by the programmer."

I guess if we were willing to double the number of definitions for
OptionalStorage once more, we might be able to not declare constructors for Ts
with trivial default constructors, but there would be no point in doing that
since the OptionalStorage default constructor is going to be nontrivial anyway
because we need to initialize is_null_.

+  // When T is trivially destructible (i.e. its destructor does nothing) there
+  // is no need to call it. Explicitly defaulting the destructor means it's not
+  // user-provided. Those two together make this destructor trivial.
   ~OptionalStorage() = default;
 
   bool is_null_ = true;
-  base::AlignedMemory<sizeof(T), ALIGNOF(T)> buffer_;
+  union {
+    // empty_ exists so that the union will always be initialized, even when it

[danakj, 2016/08/15 21:22:57]

nitto

[kwiberg-chromium, 2016/08/16 08:23:47]

Ha! Done.

+    // doesn't contain a value. Not initializing it has been observed to
+    // trigger comiler warnings.
+    char empty_ = '\0';
+    T value_;
+  };
 };
 
 }  // 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
 // Chromium documentation:
 // https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md
 //
@@ skipping 92 matching lines @@
   T&& operator*() && { return std::move(value()); }
 
   constexpr explicit operator bool() const { return !storage_.is_null_; }
 
   constexpr bool has_value() const { return !storage_.is_null_; }
 
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T& value() & {
     DCHECK(!storage_.is_null_);
-    return *storage_.buffer_.template data_as<T>();
+    return storage_.value_;
   }
 
   // TODO(mlamouri): can't use 'constexpr' with DCHECK.
   const T& value() const& {
     DCHECK(!storage_.is_null_);
-    return *storage_.buffer_.template data_as<T>();
+    return storage_.value_;
   }
 
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T&& value() && {
     DCHECK(!storage_.is_null_);
-    return std::move(*storage_.buffer_.template data_as<T>());
+    return std::move(storage_.value_);
   }
 
   // TODO(mlamouri): can't use 'constexpr' with DCHECK.
   const T&& value() const&& {
     DCHECK(!storage_.is_null_);
-    return std::move(*storage_.buffer_.template data_as<T>());
+    return std::move(storage_.value_);
   }
 
   template <class U>
   constexpr T value_or(U&& default_value) const& {
     // TODO(mlamouri): add the following assert when possible:
     // static_assert(std::is_copy_constructible<T>::value,
     //               "T must be copy constructible");
     static_assert(std::is_convertible<U, T>::value,
                   "U must be convertible to T");
     return storage_.is_null_ ? static_cast<T>(std::forward<U>(default_value))
@@ skipping 10 matching lines @@
     return storage_.is_null_ ? static_cast<T>(std::forward<U>(default_value))
                              : std::move(value());
   }
 
   void swap(Optional& other) {
     if (storage_.is_null_ && other.storage_.is_null_)
       return;
 
     if (storage_.is_null_ != other.storage_.is_null_) {
       if (storage_.is_null_) {
-        Init(std::move(*other.storage_.buffer_.template data_as<T>()));
+        Init(std::move(other.storage_.value_));
         other.FreeIfNeeded();
       } else {
-        other.Init(std::move(*storage_.buffer_.template data_as<T>()));
+        other.Init(std::move(storage_.value_));
         FreeIfNeeded();
       }
       return;
     }
 
     DCHECK(!storage_.is_null_ && !other.storage_.is_null_);
     using std::swap;
     swap(**this, *other);
   }
 
   void reset() {
     FreeIfNeeded();
   }
 
   template <class... Args>
   void emplace(Args&&... args) {
     FreeIfNeeded();
     Init(std::forward<Args>(args)...);
   }
 
  private:
   void Init(const T& value) {
     DCHECK(storage_.is_null_);
-    new (storage_.buffer_.void_data()) T(value);
+    new (&storage_.value_) T(value);
     storage_.is_null_ = false;
   }
 
   void Init(T&& value) {
     DCHECK(storage_.is_null_);
-    new (storage_.buffer_.void_data()) T(std::move(value));
+    new (&storage_.value_) T(std::move(value));
     storage_.is_null_ = false;
   }
 
   template <class... Args>
   void Init(Args&&... args) {
     DCHECK(storage_.is_null_);
-    new (storage_.buffer_.void_data()) T(std::forward<Args>(args)...);
+    new (&storage_.value_) T(std::forward<Args>(args)...);
     storage_.is_null_ = false;
   }
 
   void InitOrAssign(const T& value) {
     if (storage_.is_null_)
       Init(value);
     else
-      *storage_.buffer_.template data_as<T>() = value;
+      storage_.value_ = value;
   }
 
   void InitOrAssign(T&& value) {
     if (storage_.is_null_)
       Init(std::move(value));
     else
-      *storage_.buffer_.template data_as<T>() = std::move(value);
+      storage_.value_ = std::move(value);
   }
 
   void FreeIfNeeded() {
     if (storage_.is_null_)
       return;
-    storage_.buffer_.template data_as<T>()->~T();
+    storage_.value_.~T();
     storage_.is_null_ = true;
   }
 
   internal::OptionalStorage<T> storage_;
 };
 
 template <class T>
 constexpr bool operator==(const Optional<T>& lhs, const Optional<T>& rhs) {
   return !!lhs != !!rhs ? false : lhs == nullopt || (*lhs == *rhs);
 }
@@ skipping 160 matching lines @@
 template <class T>
 struct hash<base::Optional<T>> {
   size_t operator()(const base::Optional<T>& opt) const {
     return opt == base::nullopt ? 0 : std::hash<T>()(*opt);
   }
 };
 
 }  // namespace std
 
 #endif  // BASE_OPTIONAL_H_