[PATCH] D98477: [ADT] Add IntrusiveVariant, VariantTraits, and new STLForwardCompat

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D98477#2800058 <https://reviews.llvm.org/D98477#2800058>, @scott.linder wrote:

> In D98477#2798018 <https://reviews.llvm.org/D98477#2798018>, @dblaikie wrote:
>
>> In D98477#2797129 <https://reviews.llvm.org/D98477#2797129>, @scott.linder wrote:
>>
>>> In D98477#2787409 <https://reviews.llvm.org/D98477#2787409>, @dblaikie wrote:
>>>
>>>> In D98477#2783276 <https://reviews.llvm.org/D98477#2783276>, @scott.linder wrote:
>>>>
>>>>> In D98477#2780702 <https://reviews.llvm.org/D98477#2780702>, @dblaikie wrote:
>>>>>
>>>>>> Side thought: If this is going to use a prefix for the discriminator - could that be a base class that all types being used as union members have to derive from? No need for a macro, etc. Could use an offsetof comparison to make sure that not only is it a base class, but the first non-empty base class.
>>>>>>
>>>>>> (still haven't been able to easily apply this locally - looks like it hit a conflict with c6f20d70a8c93ab3d50c9777c477fe040460a664 <https://reviews.llvm.org/rGc6f20d70a8c93ab3d50c9777c477fe040460a664> - though I thought arc would sync to the right version to apply the patch... not sure what's going on - might just wait to try that until the other patches have been committed)
>>>>>
>>>>> Unfortunately having any non-static data members as part of a base class will make the alternative types non-standard-layout, which means we can't use the "common initial sequence" rule to pack them.
>>>>
>>>> But would we need to use the common initial sequence rule? If the base object was at the start of the layout - a pointer to the start of the layout would be a valid pointer to the base object, yeah?
>>>
>>> AFAIK that isn't true: we can't assume the base subobject is at the start of the layout of the derived object unless both are standard-layout.
>>
>> My hope/thought was/is to use offsetof to check that the member is at the start of the object. Though, yes, for maximal portability, offsetof does require standard layout (though it seems both clang and gcc support it at least in some more cases than that: https://godbolt.org/z/Pjr6YEqMq )
>>
>> In any case - even if it still leaves us with the standard layout restriction - using a base class rather than a macro+member seems like a nicer/more usable solution?
>
> The moment we have non-static data members in both a derived class and its base class the derived class becomes non-standard-layout, though. Even just the following will not work:
>
>   struct VariantHeader {
>     uint8_t Tag;
>   };
>   
>   // SomeAlternative is not standard-layout
>   struct SomeAlternative : VariantHeader {
>     int Foo;
>   };

Ah, thanks for the correction, I missed the "Has all non-static data members and bit-fields declared in the same class (either all in the derived or all in some base)" part of the Standard Layout requirement.

@rsmith got any thoughts on this? I'd rather not have a macro stamping out this member, though I appreciate wanting to have it only be accessible to the variant type (so wanting it to be private+friended) - I do rather like/prefer being able to use a base class, though it means going off-spec with offsetof usage in a non-standard-layout type (& possibly making the offsetof check only done on certain compilers - crossing our fingers that the compilers we didn't test on don't break the layout requirement))


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98477