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

Ilya Biryukov via llvm-dev llvm-dev at lists.llvm.org
Thu Jan 25 03:17:24 PST 2018


Ben's change seems reasonable and also +1 to making non-trivial types
behave like std::optional.
We can workaround our failures.

On Thu, Jan 25, 2018 at 12:57 AM, Sam McCall <sammccall at google.com> wrote:

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


-- 
Regards,
Ilya Biryukov
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180125/2b080591/attachment.html>


More information about the llvm-dev mailing list