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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/AST/DeclCXX.cpp:487
   // Only calculate hash on first call of getODRHash per record.
-  ODRHash Hash;
+  class ODRHash Hash;
   Hash.AddCXXRecordDecl(getDefinition());
----------------
I think this change is no longer necessary.


================
Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+    assert(getContext().hasSameType(FirstField->getType(),
+                                    SecondField->getType()));
+
----------------
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?


================
Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:489
+  // of CXXRecordDecl.
+  Record.push_back(Writer.getLangOpts().CPlusPlus ? 0UL : D->getODRHash());
 
----------------
It would be better to avoid writing this at all for a CXXRecordDecl, to avoid adding 6 unused bits per class to the bitcode. (Please also look at isa<CXXRecordDecl>(D) not at the LangOpts here.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71734