rsmith added inline comments.
================
Comment at: clang/include/clang/AST/Decl.h:4009-4010
+
+ /// Store the ODR hash for this decl.
+ unsigned ODRHash;
};
----------------
We shouldn't store this here; this will make all `CXXRecordDecl`s larger to
store a field they will never look at.
We have 27 spare trailing bits in `RecordDeclBitfields` (28 if you don't add
the `HasODRHash` bit and use 0 to mean "hash not computed"). Can we store this
there instead of here?
================
Comment at: clang/lib/AST/ODRHash.cpp:480-481
+ if (auto *SubRD = dyn_cast<RecordDecl>(SubDecl)) {
+ if (!SubRD->isCompleteDefinition())
+ continue;
+ ID.AddInteger(SubRD->getODRHash());
----------------
This is, at least in principle, not reliable. After definition merging, we
could have picked a different definition of this record type as "the"
definition. (It's probably not straightforward to get this to happen in
practice, and maybe not even possible, but I'm not certain of that.)
Maybe we could add to `TagDecl` something like
```
bool isThisDeclarationADemotedDefinition() const {
return !isThisDeclarationADefinition() && BraceRange.isValid();
}
```
and then check `isThisDeclarationADefinition() ||
isThisDeclarationADemotedDefinition()` here? (I'm not sure we always update
`BraceRange` properly for demoted definitions either, so maybe that's not quite
right.)
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+ assert(getContext().hasSameType(FirstField->getType(),
+ SecondField->getType()));
+
----------------
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?
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
