bruno added inline comments.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+ assert(getContext().hasSameType(FirstField->getType(),
+ SecondField->getType()));
+
----------------
rsmith wrote:
> bruno wrote:
> > rsmith wrote:
> > > I don't understand why this assertion would be correct if the above
> > > branch can ever be taken. It's possible for two different types to have
> > > the same hash, and it looks like we'll assert here if that happens.
> > > Should we be branching on `hasSameType` above instead of comparing hashes?
> > This is trying to cover two things. The first one is to handle nested
> > anonymous unions, which might have the same type, but underlying
> > mismatching fields:
> > ```
> > #if defined(FIRST)
> > struct AU {
> > union {
> > int a;
> > };
> > };
> > #elif defined(SECOND)
> > struct AU {
> > union {
> > char a;
> > };
> > };
> > #else
> > struct AU au;
> > ```
> >
> > The second is to allow for @interfaces (downstream patches at the moment)
> > to reuse logic to diagnose fields in `ODRDiagField` without having to add
> > its own check for the underlying types. Does that makes sense?
> I still don't understand.
>
> Either the types are always the same before the previous `if` and can only
> differ in type sugar (in which case this change is unnecessary), or the types
> can be different in more than just sugar (in which case this assert is wrong
> because there's no guarantee that different types will have different hashes).
>
> What am I missing?
@vsapsai: just to follow up here a bit. I don't remember the full context
anymore, but it should be fine to reintroduce this in a later patch with a
better explanation to @rsmith, or change the approach if it makes sense. Thanks
for taking this over!
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D71734/new/
https://reviews.llvm.org/D71734
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
