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

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>

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
>
>

On Wed, Jan 24, 2018 at 11:47 PM, Benjamin Kramer <benny.kra at gmail.com>
wrote:
> 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?
It's not bad, there's other options. I'd be more worried about that
change
subtly breaking existing usages.

 The use case is objects with side-effecty destructors that should only run
once when the "real" object is moved.
MakeCleanup
<https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/lib/gtl/cleanup.h>
is
the archetype here.

my options are:
a) wrap the object in optional - this requires the "moved = None"
semantics
b) wrap the object in unique_ptr - this is heap allocation for no real
reason
c) write the move constructor/assignment to track an "IsMoved" flag -
being
able to "= default" is easier to get right
d) write a utility specifically for this idiom, e.g. a move-tracking
boolean or a UniqueOptional that adds those semantics.

In practice unique_ptr is the easiest thing and I don't care about the
allocation.

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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20180125/e4a8ff96/attachment.html>