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