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 cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits