[PATCH] D110287: [modules] While merging ObjCInterfaceDecl definitions, merge them as decl contexts too.

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith accepted this revision.
rsmith added inline comments.
This revision is now accepted and ready to land.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184
+  if (DD.Definition != NewDD.Definition) {
+    Reader.MergedDeclContexts.insert(
+        std::make_pair(NewDD.Definition, DD.Definition));
+  }
----------------
vsapsai wrote:
> rsmith wrote:
> > A couple of other things you should consider doing in this case:
> > 
> > * If there's an AST invariant that there is only one definition, think about how to maintain that invariant. For other kinds of declaration, we would update `NewDD` to make it act as a forward-declaration, but I'm not sure we can do much about it here given that an `@interface` is never a forward-declaration. Maybe for `@interface` there's nothing we can really do, and code dealing with these declarations just needs to handle there possibly being more than one definition?
> > * Merge the definition visibility of the new definition into the canonical definition. This is necessary to ensure that in code for which the new definition is imported and the canonical definition is not, we still treat the canonical definition as being visible. (Eg, module A has the canonical definition, module B has another definition, module C imports A but doesn't re-export it, and C also imports B and re-exports it, and then you import C and try to use the class.)
> > 
> > For the latter, `Reader.mergeDefinitionVisibility(DD.Definition, NewDD.Definition)` should be sufficient, assuming that all code treats the first definition as being the canonical one (for example, name lookups are performed into the first definition).
> Change to merge visibility is D110453 and you are right, the code is simple, the main complexity is the test.
> 
> As for maintaining a single definition, we are already doing that (see my comment a few lines below in `ASTDeclReader::VisitObjCInterfaceDecl`). But not doing good job with `ObjCInterfaceType::getDecl`. The fix for that is D110452 but the summary is that `ObjCInterfaceType` stores the last encountered definition, it is never updated when we discard this definition, that not-so-definition-anymore is treated as a canonical definition.
> 
> Updating visibility doesn't work without D110452 because we might end up looking up a different "definition", not the one with correct[ed] visibility. And D110452 seems to be different enough, so I've split it out as a separate patch. I've made current change a separate patch for clear indication what code changes lead to changes in clang/test/Modules/odr_hash.mm, there is no technical reason it cannot be merged with other changes.
> 
> Also I should note that `@interface` tracking story isn't over yet, I still need to check and write tests for merging categories. They cannot have ivars, so don't expect IRGen issues but there is still enough room for other bugs.
OK, separating out those changes with separate tests makes sense to me. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D110287/new/

https://reviews.llvm.org/D110287