[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/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