<div dir="ltr">Ben's change seems reasonable and also +1 to making non-trivial types behave like std::optional.<div>We can workaround our failures.</div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jan 25, 2018 at 12:57 AM, Sam McCall <span dir="ltr"><<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="">On Wed, Jan 24, 2018 at 11:47 PM, Benjamin Kramer <span dir="ltr"><<a href="mailto:benny.kra@gmail.com" target="_blank">benny.kra@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">That's an unintentional change. However, the reason for this change<br>
was to make optional of trivially copyable types trivially copyable,<br>
adding a user-provided move ctor would break that again :(<br>
<br>
I'm leaning towards making the non-trivial version of llvm::Optional<br>
more like std::optional. In the long term std::optional is going to<br>
replace llvm::Optional. How bad would that be for your use case?</blockquote></span><div>It's not bad, there's other options. I'd be more worried about that change subtly breaking existing usages.</div><div><br></div><div> The use case is objects with side-effecty destructors that should only run once when the "real" object is moved.</div><div><a href="https://github.com/tensorflow/tensorflow/blob/master/tensorflow/core/lib/gtl/cleanup.h" target="_blank">MakeCleanup</a> is the archetype here.</div><div><br></div><div>my options are:</div><div>a) wrap the object in optional - this requires the "moved = None" semantics</div><div>b) wrap the object in unique_ptr - this is heap allocation for no real reason</div><div>c) write the move constructor/assignment to track an "IsMoved" flag - being able to "= default" is easier to get right</div><div>d) write a utility specifically for this idiom, e.g. a move-tracking boolean or a UniqueOptional that adds those semantics. </div><div><br></div><div>In practice unique_ptr is the easiest thing and I don't care about the allocation.</div><div><div class="h5"><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-8219070306905806226HOEnZb"><div class="m_-8219070306905806226h5">
On Wed, Jan 24, 2018 at 9:20 PM, Sam McCall <<a href="mailto:sammccall@google.com" target="_blank">sammccall@google.com</a>> wrote:<br>
> Hey Ben,<br>
><br>
> This change broke some clangd code (no failing test, mea culpa), because it<br>
> changed the move semantics.<br>
><br>
> Previously: a moved-from llvm::Optional was None (for all types).<br>
> Now: a moved-from llvm::Optional is None (for non-trivial types), and has<br>
> the old value (for trivial types).<br>
><br>
> FWIW, a moved-from std::optional is *not* nullopt, and contains the<br>
> moved-from value.<br>
> This seems sad to me, and kills a nice use of optional (this one), but<br>
> there's some value in consistency with std.<br>
><br>
> Either way, I wanted to bring this up because<br>
> - I wasn't sure it was explicitly considered, and should probably be<br>
> documented<br>
> - I think we should have either the old llvm behavior or the std behavior,<br>
> the new hybrid is a gotcha I think.<br>
><br>
> WDYT?<br>
><br>
> On Thu, Jan 18, 2018 at 12:26 PM, Benjamin Kramer via llvm-commits<br>
> <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> Author: d0k<br>
>> Date: Thu Jan 18 03:26:24 2018<br>
>> New Revision: 322838<br>
>><br>
>> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=322838&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject?rev=322838&view=rev</a><br>
>> Log:<br>
>> [ADT] Split optional to only include copy mechanics and dtor for<br>
>> non-trivial types.<br>
>><br>
>> This makes uses of Optional more transparent to the compiler (and<br>
>> clang-tidy) and generates slightly smaller code.<br>
>><br>
>> This is a re-land of r317019, which had issues with GCC 4.8 back then.<br>
>> Those issues don't reproduce anymore, but I'll watch the buildbots<br>
>> closely in case anything goes wrong.<br>
>><br>
>> Modified:<br>
>> llvm/trunk/include/llvm/ADT/O<wbr>ptional.h<br>
>> llvm/trunk/unittests/ADT/Opti<wbr>onalTest.cpp<br>
>><br>
>> Modified: llvm/trunk/include/llvm/ADT/Op<wbr>tional.h<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/Optional.h?rev=322838&r1=322837&r2=322838&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/include/llvm/<wbr>ADT/Optional.h?rev=322838&r1=<wbr>322837&r2=322838&view=diff</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- llvm/trunk/include/llvm/ADT/Op<wbr>tional.h (original)<br>
>> +++ llvm/trunk/include/llvm/ADT/Op<wbr>tional.h Thu Jan 18 03:26:24 2018<br>
>> @@ -27,139 +27,173 @@<br>
>><br>
>> namespace llvm {<br>
>><br>
>> -template <typename T> class Optional {<br>
>> +namespace optional_detail {<br>
>> +/// Storage for any type.<br>
>> +template <typename T, bool IsPodLike> struct OptionalStorage {<br>
>> AlignedCharArrayUnion<T> storage;<br>
>> bool hasVal = false;<br>
>><br>
>> -public:<br>
>> - using value_type = T;<br>
>> -<br>
>> - Optional(NoneType) {}<br>
>> - explicit Optional() {}<br>
>> -<br>
>> - Optional(const T &y) : hasVal(true) { new (storage.buffer) T(y); }<br>
>> + OptionalStorage() = default;<br>
>><br>
>> - Optional(const Optional &O) : hasVal(O.hasVal) {<br>
>> + OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y);<br>
>> }<br>
>> + OptionalStorage(const OptionalStorage &O) : hasVal(O.hasVal) {<br>
>> if (hasVal)<br>
>> - new (storage.buffer) T(*O);<br>
>> + new (storage.buffer) T(*O.getPointer());<br>
>> }<br>
>> -<br>
>> - Optional(T &&y) : hasVal(true) { new (storage.buffer)<br>
>> T(std::forward<T>(y)); }<br>
>> -<br>
>> - Optional(Optional<T> &&O) : hasVal(O) {<br>
>> - if (O) {<br>
>> - new (storage.buffer) T(std::move(*O));<br>
>> + OptionalStorage(T &&y) : hasVal(true) {<br>
>> + new (storage.buffer) T(std::forward<T>(y));<br>
>> + }<br>
>> + OptionalStorage(OptionalStorag<wbr>e &&O) : hasVal(O.hasVal) {<br>
>> + if (O.hasVal) {<br>
>> + new (storage.buffer) T(std::move(*O.getPointer()));<br>
>> O.reset();<br>
>> }<br>
>> }<br>
>><br>
>> - ~Optional() { reset(); }<br>
>> -<br>
>> - Optional &operator=(T &&y) {<br>
>> + OptionalStorage &operator=(T &&y) {<br>
>> if (hasVal)<br>
>> - **this = std::move(y);<br>
>> + *getPointer() = std::move(y);<br>
>> else {<br>
>> new (storage.buffer) T(std::move(y));<br>
>> hasVal = true;<br>
>> }<br>
>> return *this;<br>
>> }<br>
>> -<br>
>> - Optional &operator=(Optional &&O) {<br>
>> - if (!O)<br>
>> + OptionalStorage &operator=(OptionalStorage &&O) {<br>
>> + if (!O.hasVal)<br>
>> reset();<br>
>> else {<br>
>> - *this = std::move(*O);<br>
>> + *this = std::move(*O.getPointer());<br>
>> O.reset();<br>
>> }<br>
>> return *this;<br>
>> }<br>
>><br>
>> - /// Create a new object by constructing it in place with the given<br>
>> arguments.<br>
>> - template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {<br>
>> - reset();<br>
>> - hasVal = true;<br>
>> - new (storage.buffer) T(std::forward<ArgTypes>(Args)<wbr>...);<br>
>> - }<br>
>> -<br>
>> - static inline Optional create(const T *y) {<br>
>> - return y ? Optional(*y) : Optional();<br>
>> - }<br>
>> -<br>
>> // FIXME: these assignments (& the equivalent const T&/const Optional&<br>
>> ctors)<br>
>> // could be made more efficient by passing by value, possibly unifying<br>
>> them<br>
>> // with the rvalue versions above - but this could place a different<br>
>> set of<br>
>> // requirements (notably: the existence of a default ctor) when<br>
>> implemented<br>
>> // in that way. Careful SFINAE to avoid such pitfalls would be<br>
>> required.<br>
>> - Optional &operator=(const T &y) {<br>
>> + OptionalStorage &operator=(const T &y) {<br>
>> if (hasVal)<br>
>> - **this = y;<br>
>> + *getPointer() = y;<br>
>> else {<br>
>> new (storage.buffer) T(y);<br>
>> hasVal = true;<br>
>> }<br>
>> return *this;<br>
>> }<br>
>> -<br>
>> - Optional &operator=(const Optional &O) {<br>
>> - if (!O)<br>
>> + OptionalStorage &operator=(const OptionalStorage &O) {<br>
>> + if (!O.hasVal)<br>
>> reset();<br>
>> else<br>
>> - *this = *O;<br>
>> + *this = *O.getPointer();<br>
>> return *this;<br>
>> }<br>
>><br>
>> + ~OptionalStorage() { reset(); }<br>
>> +<br>
>> void reset() {<br>
>> if (hasVal) {<br>
>> - (**this).~T();<br>
>> + (*getPointer()).~T();<br>
>> hasVal = false;<br>
>> }<br>
>> }<br>
>><br>
>> - const T *getPointer() const {<br>
>> - assert(hasVal);<br>
>> - return reinterpret_cast<const T *>(storage.buffer);<br>
>> - }<br>
>> T *getPointer() {<br>
>> assert(hasVal);<br>
>> return reinterpret_cast<T *>(storage.buffer);<br>
>> }<br>
>> - const T &getValue() const LLVM_LVALUE_FUNCTION {<br>
>> + const T *getPointer() const {<br>
>> assert(hasVal);<br>
>> - return *getPointer();<br>
>> + return reinterpret_cast<const T *>(storage.buffer);<br>
>> }<br>
>> - T &getValue() LLVM_LVALUE_FUNCTION {<br>
>> - assert(hasVal);<br>
>> - return *getPointer();<br>
>> +};<br>
>> +<br>
>> +/// Storage for trivially copyable types only.<br>
>> +template <typename T> struct OptionalStorage<T, true> {<br>
>> + AlignedCharArrayUnion<T> storage;<br>
>> + bool hasVal = false;<br>
>> +<br>
>> + OptionalStorage() = default;<br>
>> +<br>
>> + OptionalStorage(const T &y) : hasVal(true) { new (storage.buffer) T(y);<br>
>> }<br>
>> + OptionalStorage &operator=(const T &y) {<br>
>> + new (storage.buffer) T(y);<br>
>> + hasVal = true;<br>
>> + return *this;<br>
>> }<br>
>><br>
>> - explicit operator bool() const { return hasVal; }<br>
>> - bool hasValue() const { return hasVal; }<br>
>> - const T *operator->() const { return getPointer(); }<br>
>> - T *operator->() { return getPointer(); }<br>
>> - const T &operator*() const LLVM_LVALUE_FUNCTION {<br>
>> - assert(hasVal);<br>
>> - return *getPointer();<br>
>> + void reset() { hasVal = false; }<br>
>> +};<br>
>> +} // namespace optional_detail<br>
>> +<br>
>> +template <typename T> class Optional {<br>
>> + optional_detail::OptionalStora<wbr>ge<T, isPodLike<T>::value> Storage;<br>
>> +<br>
>> +public:<br>
>> + using value_type = T;<br>
>> +<br>
>> + constexpr Optional() {}<br>
>> + constexpr Optional(NoneType) {}<br>
>> +<br>
>> + Optional(const T &y) : Storage(y) {}<br>
>> + Optional(const Optional &O) = default;<br>
>> +<br>
>> + Optional(T &&y) : Storage(std::forward<T>(y)) {}<br>
>> + Optional(Optional &&O) = default;<br>
>> +<br>
>> + Optional &operator=(T &&y) {<br>
>> + Storage = std::move(y);<br>
>> + return *this;<br>
>> }<br>
>> - T &operator*() LLVM_LVALUE_FUNCTION {<br>
>> - assert(hasVal);<br>
>> - return *getPointer();<br>
>> + Optional &operator=(Optional &&O) = default;<br>
>> +<br>
>> + /// Create a new object by constructing it in place with the given<br>
>> arguments.<br>
>> + template <typename... ArgTypes> void emplace(ArgTypes &&... Args) {<br>
>> + reset();<br>
>> + Storage.hasVal = true;<br>
>> + new (getPointer()) T(std::forward<ArgTypes>(Args)<wbr>...);<br>
>> }<br>
>><br>
>> + static inline Optional create(const T *y) {<br>
>> + return y ? Optional(*y) : Optional();<br>
>> + }<br>
>> +<br>
>> + Optional &operator=(const T &y) {<br>
>> + Storage = y;<br>
>> + return *this;<br>
>> + }<br>
>> + Optional &operator=(const Optional &O) = default;<br>
>> +<br>
>> + void reset() { Storage.reset(); }<br>
>> +<br>
>> + const T *getPointer() const {<br>
>> + assert(Storage.hasVal);<br>
>> + return reinterpret_cast<const T *>(Storage.storage.buffer);<br>
>> + }<br>
>> + T *getPointer() {<br>
>> + assert(Storage.hasVal);<br>
>> + return reinterpret_cast<T *>(Storage.storage.buffer);<br>
>> + }<br>
>> + const T &getValue() const LLVM_LVALUE_FUNCTION { return *getPointer();<br>
>> }<br>
>> + T &getValue() LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
>> +<br>
>> + explicit operator bool() const { return Storage.hasVal; }<br>
>> + bool hasValue() const { return Storage.hasVal; }<br>
>> + const T *operator->() const { return getPointer(); }<br>
>> + T *operator->() { return getPointer(); }<br>
>> + const T &operator*() const LLVM_LVALUE_FUNCTION { return *getPointer();<br>
>> }<br>
>> + T &operator*() LLVM_LVALUE_FUNCTION { return *getPointer(); }<br>
>> +<br>
>> template <typename U><br>
>> constexpr T getValueOr(U &&value) const LLVM_LVALUE_FUNCTION {<br>
>> return hasValue() ? getValue() : std::forward<U>(value);<br>
>> }<br>
>><br>
>> #if LLVM_HAS_RVALUE_REFERENCE_THIS<br>
>> - T &&getValue() && {<br>
>> - assert(hasVal);<br>
>> - return std::move(*getPointer());<br>
>> - }<br>
>> - T &&operator*() && {<br>
>> - assert(hasVal);<br>
>> - return std::move(*getPointer());<br>
>> - }<br>
>> + T &&getValue() && { return std::move(*getPointer()); }<br>
>> + T &&operator*() && { return std::move(*getPointer()); }<br>
>><br>
>> template <typename U><br>
>> T getValueOr(U &&value) && {<br>
>><br>
>> Modified: llvm/trunk/unittests/ADT/Optio<wbr>nalTest.cpp<br>
>> URL:<br>
>> <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/OptionalTest.cpp?rev=322838&r1=322837&r2=322838&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-pr<wbr>oject/llvm/trunk/unittests/ADT<wbr>/OptionalTest.cpp?rev=322838&<wbr>r1=322837&r2=322838&view=diff</a><br>
>><br>
>> ==============================<wbr>==============================<wbr>==================<br>
>> --- llvm/trunk/unittests/ADT/Optio<wbr>nalTest.cpp (original)<br>
>> +++ llvm/trunk/unittests/ADT/Optio<wbr>nalTest.cpp Thu Jan 18 03:26:24 2018<br>
>> @@ -518,5 +518,14 @@ TEST_F(OptionalTest, OperatorGreaterEqua<br>
>> CheckRelation<GreaterEqual>(In<wbr>equalityLhs, InequalityRhs, !IsLess);<br>
>> }<br>
>><br>
>> +#if (__has_feature(is_trivially_co<wbr>pyable) && defined(_LIBCPP_VERSION)) ||<br>
>> \<br>
>> + (defined(__GNUC__) && __GNUC__ >= 5)<br>
>> +static_assert(std::is_trivial<wbr>ly_copyable<Optional<int>>::<wbr>value,<br>
>> + "Should be trivially copyable");<br>
>> +static_assert(<br>
>> +<br>
>> !std::is_trivially_copyable<Op<wbr>tional<NonDefaultConstructible<wbr>>>::value,<br>
>> + "Shouldn't be trivially copyable");<br>
>> +#endif<br>
>> +<br>
>> } // end anonymous namespace<br>
>><br>
>><br>
>><br>
>> ______________________________<wbr>_________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
><br>
><br>
</div></div></blockquote></div></div></div><br></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div>Regards,</div><div>Ilya Biryukov</div></div></div></div></div>
</div>