bruno marked 3 inline comments as done.
bruno added a comment.
Thanks for taking a look Richard, comments inline.
================
Comment at: clang/include/clang/AST/Decl.h:4009-4010
+
+ /// Store the ODR hash for this decl.
+ unsigned ODRHash;
};
----------------
rsmith wrote:
> 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?
Makes sense!
================
Comment at: clang/lib/AST/ODRHash.cpp:480-481
+ if (auto *SubRD = dyn_cast<RecordDecl>(SubDecl)) {
+ if (!SubRD->isCompleteDefinition())
+ continue;
+ ID.AddInteger(SubRD->getODRHash());
----------------
rsmith wrote:
> 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.)
There are two things I realized while looking at this:
- What I really intended here was to hash underlying anonymous structs/unions,
not just any underlying RecordDecl. Will change the logic in
`ODRHash::AddRecordDecl` do reflect that properly.
- This patch was skipping `RecordDecl`s without definitions during ODR
diagnostics, I've changed to instead decide at merge time if we want to
register those Decls for later diagnose, and the new rule is clang skips
`RecordDecl`s that disagree on whether they have a definition. This should
still allow clang to diagnose declarations without defintions that have
different attributes (which I intend to add for specific attributes in future
patches).
Will update the patch to reflect that. WDYT?
================
Comment at: clang/lib/Serialization/ASTReader.cpp:9585-9587
+ assert(getContext().hasSameType(FirstField->getType(),
+ SecondField->getType()));
+
----------------
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?
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
