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 {
+    T value_;

[danakj, 2016/06/22 23:00:03]

Why does libcxx put a second field in the union?
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...

And then initialize it instead in the default constructor:
https://github.com/llvm-mirror/libcxx/blob/master/include/experimental/option...

[kwiberg-chromium, 2016/06/23 02:18:49]

It seems like it's required to make constexpr construction work fully:
https://akrzemi1.wordpress.com/2012/12/13/constexpr-unions/. (The code described
in that article isn't identical to what's in libcxx, though, but it seems to me
like they'd work almost the same.)

But the implementation here doesn't try to provide constexpr constructors. Or so
it claims---but I see an awful lot of "constexpr"s in this file...

I think adding an extra dummy member can probably wait until there are unit
tests that try to prove that constexpr construction works; until then, it'd just
be useless noise that maybe doesn't even work because it isn't being tested.

+  };
 };
 
 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() {};
+  // 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 {
+    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 90 matching lines @@
   // TODO(mlamouri): using 'constexpr' here breaks compiler that assume it was
   // meant to be 'constexpr const'.
   T&& operator*() && { return std::move(value()); }
 
   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(!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);
   }
 
   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_