[PATCH] D71734: [Modules] Handle tag types and complain about bad merges in C/Objective-C mode

[Bruno Cardoso Lopes via Phabricator via cfe-commits]

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