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

Because it's simpler, and accomplishes the same thing. Also modify the
C++11 allowed features list to allow unions with class members, to
make this change style guide compliant. :-)

This is adapted from https://codereview.webrtc.org/2071003003/ by
ossu@webrtc.org (which adapted the technique from WebRTC's
rtc::Optional, where it has been in use since May 9:
https://crrev.com/d040480f69cc6fe65dd101c493d0561a0cdbaa4a).

Review-Url: https://codereview.chromium.org/2080003002
Cr-Commit-Position: refs/heads/master@{#412199}

base/optional.h

@@ -8,7 +8,6 @@
 #include <type_traits>
 
 #include "base/logging.h"
-#include "base/memory/aligned_memory.h"
 #include "base/template_util.h"
 
 namespace base {
@@ -35,28 +34,40 @@ 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 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() {}
+  // 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 doesn't contain a value. Not initializing it has been observed to
+    // trigger comiler warnings.
+    char empty_ = '\0';
+    T value_;
+  };
 };
 
 }  // namespace internal
@@ -169,26 +180,26 @@ class Optional {
   // 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>
@@ -219,10 +230,10 @@ class Optional {
 
     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;
@@ -246,20 +257,20 @@ class Optional {
  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;
   }
 
@@ -267,20 +278,20 @@ class Optional {
     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;
   }
 

docs/optional.md

@@ -76,7 +76,7 @@ Finally, `base::Optional<T>` is integrated with `std::hash`, using
 
 ## How is it implemented?
 
-`base::Optional<T>` is implemented using `base::AlignedMemory`. The object
+`base::Optional<T>` is implemented with a union with a `T` member. The object
 doesn't behave like a pointer and doesn't do dynamic memory allocation. In
 other words, it is guaranteed to have an object allocated when it is not empty.
 

styleguide/c++/c++11.html

@@ -286,6 +286,14 @@ template &lt;typename T&gt;<br/>void Function(T&& t) { ... }</code></td>
 <td>See the <a href="https://www.chromium.org/developers/coding-style/cpp-dos-and-donts?pli=1#TOC-Variable-initialization">Chromium C++ Dos And Don'ts guidance</a> on when to use this. <a href="https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/GF96FshwHLw">Discussion thread</a></td>
 </tr>
 
+<tr>
+<td>Union Class Members</td>
+<td><code>union <i>name</i> {<i>class</i> <i>var</i>}</code></td>
+<td>Allows class type members</td>
+<td><a href="http://en.cppreference.com/w/cpp/language/union">Union declarations</a></td>
+<td>Usage should be rare.</td>
+</tr>
+
 <tr>
 <td>Variadic Macros</td>
 <td><code>#define <i>MACRO</i>(...) <i>Impl</i>(<i>args</i>, __VA_ARGS__)</code></td>
@@ -678,14 +686,6 @@ work in all our compilers yet.</p>
 <td>Unclear how it will work with components</td>
 </tr>
 
-<tr>
-<td>Union Class Members</td>
-<td><code>union <i>name</i> {<i>class</i> <i>var</i>}</code></td>
-<td>Allows class type members</td>
-<td><a href="http://en.cppreference.com/w/cpp/language/union">Union declarations</a></td>
-<td></td>
-</tr>
-
 <tr>
 <td>User-Defined Literals</td>
 <td><code><i>type</i> <i>var</i> = <i>literal_value</i>_<i>type</i></code></td>