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

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

rsmith added inline comments.


================
Comment at: clang/lib/Serialization/ASTReaderDecl.cpp:1181-1184
+  if (DD.Definition != NewDD.Definition) {
+    Reader.MergedDeclContexts.insert(
+        std::make_pair(NewDD.Definition, DD.Definition));
+  }
----------------
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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110287