[llvm] r322838 - [ADT] Split optional to only include copy mechanics and dtor for non-trivial types.

Benjamin Kramer via llvm-commits llvm-commits at lists.llvm.org
Thu Jan 18 03:26:24 PST 2018


Author: d0k
Date: Thu Jan 18 03:26:24 2018
New Revision: 322838

URL: http://llvm.org/viewvc/llvm-project?rev=322838&view=rev
Log:
[ADT] Split optional to only include copy mechanics and dtor for non-trivial types.

This makes uses of Optional more transparent to the compiler (and
clang-tidy) and generates slightly smaller code.

This is a re-land of r317019, which had issues with GCC 4.8 back then.
Those issues don't reproduce anymore, but I'll watch the buildbots
closely in case anything goes wrong.

Modified:
    llvm/trunk/include/llvm/ADT/Optional.h
    llvm/trunk/unittests/ADT/OptionalTest.cpp

Modified: llvm/trunk/include/llvm/ADT/Optional.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Optional.h?rev=322838&r1=322837&r2=322838&view=diff
==============================================================================
--- llvm/trunk/include/llvm/ADT/Optional.h (original)
+++ llvm/trunk/include/llvm/ADT/Optional.h Thu Jan 18 03:26:24 2018
@@ -27,139 +27,173 @@
 
 namespace llvm {
 
-template <typename T> class Optional {
+namespace optional_detail {
+/// Storage for any type.
+template <typename T, bool IsPodLike> struct OptionalStorage {
   AlignedCharArrayUnion<T> storage;
   bool hasVal = false;
 
-public:
-  using value_type = T;
-
-  Optional(NoneType) {}
-  explicit Optional() {}
-
-  Optional(const T &y) : hasVal(true) { new (storage.buffer) T(y); }
+  OptionalStorage() = default;
 
-  Optional(const Optional &O) : hasVal(O.hasVal) {
+  OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y); }
+  OptionalStorage(const OptionalStorage &O) : hasVal(O.hasVal) {
     if (hasVal)
-      new (storage.buffer) T(*O);
+      new (storage.buffer) T(*O.getPointer());
   }
-
-  Optional(T &&y) : hasVal(true) { new (storage.buffer) T(std::forward<T>(y)); }
-
-  Optional(Optional<T> &&O) : hasVal(O) {
-    if (O) {
-      new (storage.buffer) T(std::move(*O));
+  OptionalStorage(T &&y) : hasVal(true) {
+    new (storage.buffer) T(std::forward<T>(y));
+  }
+  OptionalStorage(OptionalStorage &&O) : hasVal(O.hasVal) {
+    if (O.hasVal) {
+      new (storage.buffer) T(std::move(*O.getPointer()));
       O.reset();
     }
   }
 
-  ~Optional() { reset(); }
-
-  Optional &operator=(T &&y) {
+  OptionalStorage &operator=(T &&y) {
     if (hasVal)
-      **this = std::move(y);
+      *getPointer() = std::move(y);
     else {
       new (storage.buffer) T(std::move(y));
       hasVal = true;
     }
     return *this;
   }
-
-  Optional &operator=(Optional &&O) {
-    if (!O)
+  OptionalStorage &operator=(OptionalStorage &&O) {
+    if (!O.hasVal)
       reset();
     else {
-      *this = std::move(*O);
+      *this = std::move(*O.getPointer());
       O.reset();
     }
     return *this;
   }
 
-  /// Create a new object by constructing it in place with the given arguments.
-  template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {
-    reset();
-    hasVal = true;
-    new (storage.buffer) T(std::forward<ArgTypes>(Args)...);
-  }
-
-  static inline Optional create(const T *y) {
-    return y ? Optional(*y) : Optional();
-  }
-
   // FIXME: these assignments (& the equivalent const T&/const Optional& ctors)
   // could be made more efficient by passing by value, possibly unifying them
   // with the rvalue versions above - but this could place a different set of
   // requirements (notably: the existence of a default ctor) when implemented
   // in that way. Careful SFINAE to avoid such pitfalls would be required.
-  Optional &operator=(const T &y) {
+  OptionalStorage &operator=(const T &y) {
     if (hasVal)
-      **this = y;
+      *getPointer() = y;
     else {
       new (storage.buffer) T(y);
       hasVal = true;
     }
     return *this;
   }
-
-  Optional &operator=(const Optional &O) {
-    if (!O)
+  OptionalStorage &operator=(const OptionalStorage &O) {
+    if (!O.hasVal)
       reset();
     else
-      *this = *O;
+      *this = *O.getPointer();
     return *this;
   }
 
+  ~OptionalStorage() { reset(); }
+
   void reset() {
     if (hasVal) {
-      (**this).~T();
+      (*getPointer()).~T();
       hasVal = false;
     }
   }
 
-  const T *getPointer() const {
-    assert(hasVal);
-    return reinterpret_cast<const T *>(storage.buffer);
-  }
   T *getPointer() {
     assert(hasVal);
     return reinterpret_cast<T *>(storage.buffer);
   }
-  const T &getValue() const LLVM_LVALUE_FUNCTION {
+  const T *getPointer() const {
     assert(hasVal);
-    return *getPointer();
+    return reinterpret_cast<const T *>(storage.buffer);
   }
-  T &getValue() LLVM_LVALUE_FUNCTION {
-    assert(hasVal);
-    return *getPointer();
+};
+
+/// Storage for trivially copyable types only.
+template <typename T> struct OptionalStorage<T, true> {
+  AlignedCharArrayUnion<T> storage;
+  bool hasVal = false;
+
+  OptionalStorage() = default;
+
+  OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y); }
+  OptionalStorage &operator=(const T &y) {
+    new (storage.buffer) T(y);
+    hasVal = true;
+    return *this;
   }
 
-  explicit operator bool() const { return hasVal; }
-  bool hasValue() const { return hasVal; }
-  const T *operator->() const { return getPointer(); }
-  T *operator->() { return getPointer(); }
-  const T &operator*() const LLVM_LVALUE_FUNCTION {
-    assert(hasVal);
-    return *getPointer();
+  void reset() { hasVal = false; }
+};
+} // namespace optional_detail
+
+template <typename T> class Optional {
+  optional_detail::OptionalStorage<T, isPodLike<T>::value> Storage;
+
+public:
+  using value_type = T;
+
+  constexpr Optional() {}
+  constexpr Optional(NoneType) {}
+
+  Optional(const T &y) : Storage(y) {}
+  Optional(const Optional &O) = default;
+
+  Optional(T &&y) : Storage(std::forward<T>(y)) {}
+  Optional(Optional &&O) = default;
+
+  Optional &operator=(T &&y) {
+    Storage = std::move(y);
+    return *this;
   }
-  T &operator*() LLVM_LVALUE_FUNCTION {
-    assert(hasVal);
-    return *getPointer();
+  Optional &operator=(Optional &&O) = default;
+
+  /// Create a new object by constructing it in place with the given arguments.
+  template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {
+    reset();
+    Storage.hasVal = true;
+    new (getPointer()) T(std::forward<ArgTypes>(Args)...);
   }
 
+  static inline Optional create(const T *y) {
+    return y ? Optional(*y) : Optional();
+  }
+
+  Optional &operator=(const T &y) {
+    Storage = y;
+    return *this;
+  }
+  Optional &operator=(const Optional &O) = default;
+
+  void reset() { Storage.reset(); }
+
+  const T *getPointer() const {
+    assert(Storage.hasVal);
+    return reinterpret_cast<const T *>(Storage.storage.buffer);
+  }
+  T *getPointer() {
+    assert(Storage.hasVal);
+    return reinterpret_cast<T *>(Storage.storage.buffer);
+  }
+  const T &getValue() const LLVM_LVALUE_FUNCTION { return *getPointer(); }
+  T &getValue() LLVM_LVALUE_FUNCTION { return *getPointer(); }
+
+  explicit operator bool() const { return Storage.hasVal; }
+  bool hasValue() const { return Storage.hasVal; }
+  const T *operator->() const { return getPointer(); }
+  T *operator->() { return getPointer(); }
+  const T &operator*() const LLVM_LVALUE_FUNCTION { return *getPointer(); }
+  T &operator*() LLVM_LVALUE_FUNCTION { return *getPointer(); }
+
   template <typename U>
   constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {
     return hasValue() ? getValue() : std::forward<U>(value);
   }
 
 #if LLVM_HAS_RVALUE_REFERENCE_THIS
-  T &&getValue() && {
-    assert(hasVal);
-    return std::move(*getPointer());
-  }
-  T &&operator*() && {
-    assert(hasVal);
-    return std::move(*getPointer());
-  }
+  T &&getValue() && { return std::move(*getPointer()); }
+  T &&operator*() && { return std::move(*getPointer()); }
 
   template <typename U>
   T getValueOr(U &&value) && {

Modified: llvm/trunk/unittests/ADT/OptionalTest.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/OptionalTest.cpp?rev=322838&r1=322837&r2=322838&view=diff
==============================================================================
--- llvm/trunk/unittests/ADT/OptionalTest.cpp (original)
+++ llvm/trunk/unittests/ADT/OptionalTest.cpp Thu Jan 18 03:26:24 2018
@@ -518,5 +518,14 @@ TEST_F(OptionalTest, OperatorGreaterEqua
   CheckRelation<GreaterEqual>(InequalityLhs, InequalityRhs, !IsLess);
 }
 
+#if (__has_feature(is_trivially_copyable) && defined(_LIBCPP_VERSION)) ||      \
+    (defined(__GNUC__) && __GNUC__ >= 5)
+static_assert(std::is_trivially_copyable<Optional<int>>::value,
+              "Should be trivially copyable");
+static_assert(
+    !std::is_trivially_copyable<Optional<NonDefaultConstructible>>::value,
+    "Shouldn't be trivially copyable");
+#endif
+
 } // end anonymous namespace
 




More information about the llvm-commits mailing list