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

Benjamin Kramer via llvm-dev llvm-dev at lists.llvm.org
Wed Jan 24 14:47:38 PST 2018


That's an unintentional change. However, the reason for this change
was to make optional of trivially copyable types trivially copyable,
adding a user-provided move ctor would break that again :(

I'm leaning towards making the non-trivial version of llvm::Optional
more like std::optional. In the long term std::optional is going to
replace llvm::Optional. How bad would that be for your use case?

On Wed, Jan 24, 2018 at 9:20 PM, Sam McCall <sammccall at google.com> wrote:
> 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), 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
>
>


More information about the llvm-dev mailing list