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

Sam McCall via llvm-dev llvm-dev at lists.llvm.org
Wed Jan 24 12:20:22 PST 2018


Hey Ben,

This change broke some clangd code (no failing test, mea culpa), because it
changed the move semantics.

Previously: a moved-from llvm::Optional was None (for all types).
Now: a moved-from llvm::Optional is None (for non-trivial types), and has
the old value (for trivial types).

FWIW, a moved-from std::optional is *not* nullopt, and contains the
moved-from value.
This seems sad to me, and kills a nice use of optional (this one
<https://reviews.llvm.org/source/clang-tools-extra/browse/clang-tools-extra/trunk/clangd/Function.h;323350$162>),
but there's some value in consistency with std.

Either way, I wanted to bring this up because
 - I wasn't sure it was explicitly considered, and should probably be
documented
 - I think we should have either the old llvm behavior or the std behavior,
the new hybrid is a gotcha I think.

WDYT?

On Thu, Jan 18, 2018 at 12:26 PM, Benjamin Kramer via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> 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
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180124/c6894beb/attachment.html>


More information about the llvm-dev mailing list