vsapsai 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());
----------------
rsmith wrote:
> I think this change is no longer necessary.
Reverted the change.
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+ assert(getContext().hasSameType(FirstField->getType(),
+ SecondField->getType()));
+
----------------
bruno wrote:
> 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!
I think it makes the most sense to remove the assertion. Based on the local
code, it doesn't make much sense, as equality of field names doesn't imply the
equality of field types. On the higher level, we were checking field types in a
different place, specifically we populated `PendingOdrMergeChecks` in
`ASTDeclReader::findExisting` and diagnosed it earlier in this mega-method
`ASTReader::diagnoseOdrViolations` (error constant
`err_module_odr_violation_missing_decl`). Also you can notice that the
diagnostic for
```lang=c++
struct S { int x; };
struct S { float x; };
```
is different, it is
> 'S::x' from module 'SecondModule' is not present in definition of 'S' in
> module 'FirstModule'
compared to
> 'S' has different definitions in different modules; first difference is
> definition in module 'FirstModule' found field 'x' with type 'int'
that we emit in ODRDiagField.
It is possible to try to make unions behave the same way as structs (and keep
the assertion) but I'm not sure it is worth it. Looks like the major difference
is that not all tag types are true Redeclerable and I don't know if it is
something we should change.
================
Comment at: clang/lib/Serialization/ASTWriterDecl.cpp:489
+ // of CXXRecordDecl.
+ Record.push_back(Writer.getLangOpts().CPlusPlus ? 0UL : D->getODRHash());
----------------
rsmith wrote:
> 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.)
Changed to write ODR hash for everything except CXXRecordDecl (right now
everything is RecordDecl). Also updated ASTReaderDecl.cpp. Not 100% sure I
understand your comment correctly, so if my change is way off, please let me
know.
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
