[PATCH] D53755: [ASTImporter] Remove import of definition from GetAlreadyImportedOrNull

[Shafik Yaghmour via Phabricator via cfe-commits]

shafik requested changes to this revision.
shafik added a comment.
This revision now requires changes to proceed.

The rest of the changes look good.



================
Comment at: lib/AST/ExternalASTMerger.cpp:147
       ToTag->setHasExternalLexicalStorage();
-      ToTag->setMustBuildLookupTable();
+      ToTag->getPrimaryContext()->setMustBuildLookupTable();
       assert(Parent.CanComplete(ToTag));
----------------
This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` so should probably be in another PR


================
Comment at: lib/AST/ExternalASTMerger.cpp:154
       ToContainer->setHasExternalLexicalStorage();
-      ToContainer->setMustBuildLookupTable();
+      ToContainer->getPrimaryContext()->setMustBuildLookupTable();
       assert(Parent.CanComplete(ToContainer));
----------------
martong wrote:
> a_sidorin wrote:
> > Do we have to do the same for NamespaceDecls?
> Yes, I think so.
> The primary context of a `NamespaceDecl` can be other than itself:
> ```
> DeclContext *DeclContext::getPrimaryContext() {
>   switch (DeclKind) {
>   case Decl::TranslationUnit:
>   case Decl::ExternCContext:
>   case Decl::LinkageSpec:
>   case Decl::Export:
>   case Decl::Block:
>   case Decl::Captured:
>   case Decl::OMPDeclareReduction:
>     // There is only one DeclContext for these entities.
>     return this;
> 
>   case Decl::Namespace:
>     // The original namespace is our primary context.
>     return static_cast<NamespaceDecl *>(this)->getOriginalNamespace();
> ```
> Consequently, we should call `setMustBuildLookupTable` only on a `NamespaceDecl` if we know for sure that is a primary context.
This change looks unrelated to the refactoring of `GetAlreadyImportedOrNull()` so should probably be in another PR


Repository:
  rC Clang

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

https://reviews.llvm.org/D53755