[PATCH] D50119: Compiler support for P1144R0 "__is_trivially_relocatable(T)"

[Arthur O'Dwyer via Phabricator via cfe-commits]

Quuxplusone added a comment.

In https://reviews.llvm.org/D50119#1303720, @rjmccall wrote:

> In https://reviews.llvm.org/D50119#1303662, @Quuxplusone wrote:
>
> > >> I still believe it is impossible to implement `std::optional` with only `[[maybe_trivially_relocatable]]`.
> > > 
> > > This might also need elaboration.
> >
> > Because `__optional_destruct_base` contains an anonymous union member, and that union type is not destructible; therefore that union type is not trivially relocatable; therefore `__optional_destruct_base` contains a member of non-trivially-destructible type. However, I'm working on changing my patch to make anonymous unions "see-through" in this respect, so that that union's non-destructibility doesn't interfere with the `[[maybe_trivially_relocatable]]`ity of its enclosing class type, as long as all the members of the //union// are trivially relocatable. This might fix `optional`. I'm not sure yet.
>
>
> Ah, that makes sense.  And yeah, that seems like the right rule for unions.  I really appreciate you putting the effort into exploring this part of the design space.


I think it's part of the right rule, but it's not enough, either. Consider:

  struct [[clang::maybe_trivially_relocatable]] DestroyBase {
      int i;
      DestroyBase(DestroyBase&&) = delete;
      ~DestroyBase();
  };
  struct [[clang::maybe_trivially_relocatable]] MoveBase : DestroyBase {
      MoveBase(MoveBase&&);
  };
  
  static_assert(not std::is_move_constructible_v<DestroyBase>); //, therefore
  static_assert(not std::is_relocatable_v<DestroyBase>); //, therefore
  static_assert(not std::is_trivially_relocatable_v<DestroyBase>); //; and since MoveBase now has a base class of non-trivially-relocatable type,
  static_assert(not std::is_trivially_relocatable_v<MoveBase>);

So maybe you need a notion like "a class is BlahBlah if all of its direct members are BlahBlah, //and// either it is annotated `[[maybe_trivially_relocatable]]` or else it has no non-defaulted move or destroy operations." And then "a move-constructible, destructible class is trivially relocatable if it is BlahBlah, //or// if it is annotated `[[trivially_relocatable]]`." And then a class like `DestroyBase` can be "BlahBlah but not move-constructible," and if it's placed inside `MoveBase`, and `MoveBase` is //both// annotated and move-constructible, then `MoveBase` becomes trivially relocatable.

This does feel like it would be twice as hard to specify in the Standard, because I think I'd have to specify both the rules for BlahBlah //and// the rules for "trivially relocatable."

I'm also extremely concerned about the philosophical correctness of separating BlahBlahness from actual physical relocatability. What does it mean, abstractly, to say that "`DestroyBase` is not even moveable at all, but //if it were//, I'm confident that its relocation operation would be trivial"?  That's some weird counterfactual logic that forces the writer of the class (`DestroyBase` in this case, but also e.g. `lock_guard`) to think about how their class might get used downstream, and what the class's hypothetical invariants might be, under an operation (move+destroy) that it itself does not claim to support.

The benefit of my original "second-level" `[[trivially_relocatable]]` attribute is that it puts all the responsibility in a single very high-level place. `optional` is the thing that knows exactly when it's trivially relocatable (or at least can make a conservative guess, which might boil down to "I'm never trivially relocatable"), so `optional` is the thing that should get the attribute.

If I get time today I'll try adding the boolean argument to the attribute, and do the libc++ patches that way for comparison.


Repository:
  rC Clang

https://reviews.llvm.org/D50119