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

Project Infrastructure LLVM Dev List Archives
1

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?

2

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

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?

3

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

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&gt;
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.

4

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