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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
