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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D50119#1305503 <https://reviews.llvm.org/D50119#1305503>, @Quuxplusone wrote:

> In D50119#1303720 <https://reviews.llvm.org/D50119#1303720>, @rjmccall wrote:
>
> > In D50119#1303662 <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>);
>


Hmm.  I don't remember what we do in this case for `trivial_abi` (which does need to address it because it's possible to return types like `DestroyBase`).  It looks like we currently make it non-trivial.  But trivial-ABI needs to consider copy constructors as well as move constructors, which would undermine my case that it should be a subset of trivially-relocatability, at least in the fantasy world where there are correct types which default their move constructors but not their copy constructors or vice-versa.

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

I'm completely comfortable with saying that `MoveBase` just isn't trivially-relocatable under `maybe_trivially_relocatable` attribute, and if you've somehow arranged for it to be, you need to use the stronger attribute.  In practice types like this either won't exist or won't actually satisfy the requirements.  That should allow you to avoid distinguishing between `BlahBlah` and `trivially-relocatable`.  As long as the analysis jumps directly down to variant members instead of treating anonymous unions as formal subobjects, I don't think this will cause serious limitations or usability problems.  Variant members are important as a special case only because anonymous unions can't really be fixed otherwise, and it's consistent with basically every other language rule around construction and destruction.

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

Yes, it's definitely unambiguous what happens with your stronger 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.

Okay.  Sorry for the delayed response, I had last week off.


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D50119/new/

https://reviews.llvm.org/D50119