From 78f85fba22f07782b114526040654c5498fef78e Mon Sep 17 00:00:00 2001 From: Richard Smith Date: Wed, 12 Sep 2018 02:13:48 +0000 Subject: [PATCH] Track definition merging on the canonical declaration even when local submodule visibility is disabled. Attempting to pick a specific declaration to make visible when the module containing the merged declaration becomes visible is error-prone, as we don't yet know which declaration we'll choose to be the definition when we are informed of the merging. git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@342019 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/AST/ASTContext.cpp | 5 +-- lib/Sema/SemaType.cpp | 9 ++++- lib/Serialization/ASTReader.cpp | 9 +---- lib/Serialization/ASTReaderDecl.cpp | 17 ++-------- .../merge-template-pattern-visibility-3.cpp | 34 +++++++++++++++++++ 5 files changed, 46 insertions(+), 28 deletions(-) create mode 100644 test/Modules/merge-template-pattern-visibility-3.cpp diff --git a/lib/AST/ASTContext.cpp b/lib/AST/ASTContext.cpp index 0ed52ffd35..b107e8bd2a 100644 --- a/lib/AST/ASTContext.cpp +++ b/lib/AST/ASTContext.cpp @@ -932,10 +932,7 @@ void ASTContext::mergeDefinitionIntoModule(NamedDecl *ND, Module *M, if (auto *Listener = getASTMutationListener()) Listener->RedefinedHiddenDefinition(ND, M); - if (getLangOpts().ModulesLocalVisibility) - MergedDefModules[cast(ND->getCanonicalDecl())].push_back(M); - else - ND->setVisibleDespiteOwningModule(); + MergedDefModules[cast(ND->getCanonicalDecl())].push_back(M); } void ASTContext::deduplicateMergedDefinitonsFor(NamedDecl *ND) { diff --git a/lib/Sema/SemaType.cpp b/lib/Sema/SemaType.cpp index 900ccb18c7..819f9ef8a4 100644 --- a/lib/Sema/SemaType.cpp +++ b/lib/Sema/SemaType.cpp @@ -7623,8 +7623,15 @@ bool Sema::hasVisibleDefinition(NamedDecl *D, NamedDecl **Suggested, // A visible module might have a merged definition instead. if (D->isModulePrivate() ? hasMergedDefinitionInCurrentModule(D) - : hasVisibleMergedDefinition(D)) + : hasVisibleMergedDefinition(D)) { + if (CodeSynthesisContexts.empty() && + !getLangOpts().ModulesLocalVisibility) { + // Cache the fact that this definition is implicitly visible because + // there is a visible merged definition. + D->setVisibleDespiteOwningModule(); + } return true; + } return false; }; diff --git a/lib/Serialization/ASTReader.cpp b/lib/Serialization/ASTReader.cpp index b60e5f14a6..ac3b737401 100644 --- a/lib/Serialization/ASTReader.cpp +++ b/lib/Serialization/ASTReader.cpp @@ -3780,22 +3780,15 @@ void ASTReader::makeModuleVisible(Module *Mod, /// visible. void ASTReader::mergeDefinitionVisibility(NamedDecl *Def, NamedDecl *MergedDef) { - // FIXME: This doesn't correctly handle the case where MergedDef is visible - // in modules other than its owning module. We should instead give the - // ASTContext a list of merged definitions for Def. if (Def->isHidden()) { // If MergedDef is visible or becomes visible, make the definition visible. if (!MergedDef->isHidden()) Def->setVisibleDespiteOwningModule(); - else if (getContext().getLangOpts().ModulesLocalVisibility) { + else { getContext().mergeDefinitionIntoModule( Def, MergedDef->getImportedOwningModule(), /*NotifyListeners*/ false); PendingMergedDefinitionsToDeduplicate.insert(Def); - } else { - auto SubmoduleID = MergedDef->getOwningModuleID(); - assert(SubmoduleID && "hidden definition in no module"); - HiddenNamesMap[getSubmodule(SubmoduleID)].push_back(Def); } } } diff --git a/lib/Serialization/ASTReaderDecl.cpp b/lib/Serialization/ASTReaderDecl.cpp index 00fad25eea..8295107476 100644 --- a/lib/Serialization/ASTReaderDecl.cpp +++ b/lib/Serialization/ASTReaderDecl.cpp @@ -4419,22 +4419,9 @@ void ASTDeclReader::UpdateDecl(Decl *D, case UPD_DECL_EXPORTED: { unsigned SubmoduleID = readSubmoduleID(); auto *Exported = cast(D); - if (auto *TD = dyn_cast(Exported)) - Exported = TD->getDefinition(); Module *Owner = SubmoduleID ? Reader.getSubmodule(SubmoduleID) : nullptr; - if (Reader.getContext().getLangOpts().ModulesLocalVisibility) { - Reader.getContext().mergeDefinitionIntoModule(cast(Exported), - Owner); - Reader.PendingMergedDefinitionsToDeduplicate.insert( - cast(Exported)); - } else if (Owner && Owner->NameVisibility != Module::AllVisible) { - // If Owner is made visible at some later point, make this declaration - // visible too. - Reader.HiddenNamesMap[Owner].push_back(Exported); - } else { - // The declaration is now visible. - Exported->setVisibleDespiteOwningModule(); - } + Reader.getContext().mergeDefinitionIntoModule(Exported, Owner); + Reader.PendingMergedDefinitionsToDeduplicate.insert(Exported); break; } diff --git a/test/Modules/merge-template-pattern-visibility-3.cpp b/test/Modules/merge-template-pattern-visibility-3.cpp new file mode 100644 index 0000000000..9ac1b6699d --- /dev/null +++ b/test/Modules/merge-template-pattern-visibility-3.cpp @@ -0,0 +1,34 @@ +// RUN: %clang_cc1 -fmodules -emit-llvm-only %s -verify + +#pragma clang module build A +module A {} +#pragma clang module contents +#pragma clang module begin A +template void f(const T&) { T::error; } +#pragma clang module end +#pragma clang module endbuild + +#pragma clang module build B +module B {} +#pragma clang module contents +#pragma clang module begin B +template void f(const T&) { T::error; } +#pragma clang module end +#pragma clang module endbuild + +#pragma clang module build C +module C {} +#pragma clang module contents +#pragma clang module begin C +#pragma clang module load B +template void f(const T&) { T::error; } +#pragma clang module end +#pragma clang module endbuild + +#pragma clang module load A +inline void f() {} +void x() { f(); } + +#pragma clang module import C +// expected-error@* {{cannot be used prior to}} +void y(int n) { f(n); } // expected-note {{instantiation of}} -- 2.40.0