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

[Scott Linder via Phabricator via llvm-commits]

scott.linder added a comment.

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;
  };



> But yeah, that still means standard layout, and potentially using the common initial sequence rules - I think I'm OK with that. Type punning's probably OK there too, but common initial sequence seems OK. And since it's only one byte - I'm OK with going at the front rather than the end.
>
> But, yeah, I think using a base class rather than a macro would be good & I think it's as valid.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D98477