On 30 October 2017 at 15:12, Vassil Vassilev via cfe-commits < cfe-commits@lists.llvm.org> wrote:
> On 11/10/17 03:19, Richard Smith via cfe-commits wrote: > >> Author: rsmith >> Date: Tue Oct 10 18:19:11 2017 >> New Revision: 315402 >> >> URL: http://llvm.org/viewvc/llvm-project?rev=315402&view=rev >> Log: >> [modules] Only take visible using-directives into account during name >> lookup. >> >> Added: >> cfe/trunk/test/Modules/using-directive.cpp >> Modified: >> cfe/trunk/lib/Sema/SemaLookup.cpp >> >> Modified: cfe/trunk/lib/Sema/SemaLookup.cpp >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaL >> ookup.cpp?rev=315402&r1=315401&r2=315402&view=diff >> ============================================================ >> ================== >> --- cfe/trunk/lib/Sema/SemaLookup.cpp (original) >> +++ cfe/trunk/lib/Sema/SemaLookup.cpp Tue Oct 10 18:19:11 2017 >> @@ -88,13 +88,15 @@ namespace { >> /// A collection of using directives, as used by C++ unqualified >> /// lookup. >> class UnqualUsingDirectiveSet { >> + Sema &SemaRef; >> + >> typedef SmallVector<UnqualUsingEntry, 8> ListTy; >> ListTy list; >> llvm::SmallPtrSet<DeclContext*, 8> visited; >> public: >> - UnqualUsingDirectiveSet() {} >> + UnqualUsingDirectiveSet(Sema &SemaRef) : SemaRef(SemaRef) {} >> void visitScopeChain(Scope *S, Scope *InnermostFileScope) { >> // C++ [namespace.udir]p1: >> @@ -113,7 +115,8 @@ namespace { >> visit(Ctx, Ctx); >> } else if (!Ctx || Ctx->isFunctionOrMethod()) { >> for (auto *I : S->using_directives()) >> - visit(I, InnermostFileDC); >> + if (SemaRef.isVisible(I)) >> + visit(I, InnermostFileDC); >> } >> } >> } >> @@ -152,7 +155,7 @@ namespace { >> while (true) { >> for (auto UD : DC->using_directives()) { >> DeclContext *NS = UD->getNominatedNamespace(); >> - if (visited.insert(NS).second) { >> + if (visited.insert(NS).second && SemaRef.isVisible(UD)) { >> > It looks like this change breaks examples such as: > > // RUN: rm -fr %t > // RUN %clang_cc1 -fmodules -fmodules-local-submodule-visibility > -fmodules-cache-path=%t -verify %s > // expected-no-diagnostics > #pragma clang module build M > module M { module TDFNodes {} module TDFInterface {} } > #pragma clang module contents > // TDFNodes > #pragma clang module begin M.TDFNodes > namespace Detail { > namespace TDF { > class TLoopManager {}; > } > } > namespace Internal { > namespace TDF { > using namespace Detail::TDF; > } > } > #pragma clang module end > > // TDFInterface > #pragma clang module begin M.TDFInterface > #pragma clang module import M.TDFNodes > namespace Internal { > namespace TDF { > using namespace Detail::TDF; > } > } > #pragma clang module end > > #pragma clang module endbuild > > #pragma clang module import M.TDFNodes > namespace Internal { > namespace TDF { > TLoopManager * use; > } > } > > I suspect that it triggers a preexisting bug in failing to make > M.TDFNodes visible... This code in NamedDecl::declarationReplaces is wrong, we even have a FIXME for the bug: // UsingDirectiveDecl's are not really NamedDecl's, and all have same name. // They can be replaced if they nominate the same namespace. // FIXME: Is this true even if they have different module visibility? if (auto *UD = dyn_cast<UsingDirectiveDecl>(this)) return UD->getNominatedNamespace()->getOriginalNamespace() == cast<UsingDirectiveDecl>(OldD)->getNominatedNamespace() ->getOriginalNamespace(); Fixed in r316965 by deleting the above code :) This might introduce a performance regression in the presence of large numbers of duplicated using-directives. We can rectify that, if needed, by making using-directives redeclarable, but I'm not expecting that to be a problem outside of pathological cases. > addUsingDirective(UD, EffectiveDC); >> queue.push_back(NS); >> } >> @@ -1085,7 +1088,7 @@ bool Sema::CppLookupName(LookupResult &R >> // } >> // } >> // >> - UnqualUsingDirectiveSet UDirs; >> + UnqualUsingDirectiveSet UDirs(*this); >> bool VisitedUsingDirectives = false; >> bool LeftStartingScope = false; >> DeclContext *OutsideOfTemplateParamDC = nullptr; >> @@ -1868,22 +1871,19 @@ static bool LookupQualifiedNameInUsingDi >> DeclContext *StartDC) { >> assert(StartDC->isFileContext() && "start context is not a file >> context"); >> - DeclContext::udir_range UsingDirectives = >> StartDC->using_directives(); >> - if (UsingDirectives.begin() == UsingDirectives.end()) return false; >> + // We have not yet looked into these namespaces, much less added >> + // their "using-children" to the queue. >> + SmallVector<NamespaceDecl*, 8> Queue; >> // We have at least added all these contexts to the queue. >> llvm::SmallPtrSet<DeclContext*, 8> Visited; >> Visited.insert(StartDC); >> - // We have not yet looked into these namespaces, much less added >> - // their "using-children" to the queue. >> - SmallVector<NamespaceDecl*, 8> Queue; >> - >> // We have already looked into the initial namespace; seed the queue >> // with its using-children. >> - for (auto *I : UsingDirectives) { >> + for (auto *I : StartDC->using_directives()) { >> NamespaceDecl *ND = I->getNominatedNamespace()->ge >> tOriginalNamespace(); >> - if (Visited.insert(ND).second) >> + if (Visited.insert(ND).second && S.isVisible(I)) >> Queue.push_back(ND); >> } >> @@ -1931,7 +1931,7 @@ static bool LookupQualifiedNameInUsingDi >> for (auto I : ND->using_directives()) { >> NamespaceDecl *Nom = I->getNominatedNamespace(); >> - if (Visited.insert(Nom).second) >> + if (Visited.insert(Nom).second && S.isVisible(I)) >> Queue.push_back(Nom); >> } >> } >> @@ -3540,6 +3540,8 @@ static void LookupVisibleDecls(DeclConte >> if (QualifiedNameLookup) { >> ShadowContextRAII Shadow(Visited); >> for (auto I : Ctx->using_directives()) { >> + if (!Result.getSema().isVisible(I)) >> + continue; >> LookupVisibleDecls(I->getNominatedNamespace(), Result, >> QualifiedNameLookup, InBaseClass, Consumer, >> Visited, >> IncludeDependentBases); >> @@ -3746,7 +3748,7 @@ void Sema::LookupVisibleDecls(Scope *S, >> // Determine the set of using directives available during >> // unqualified name lookup. >> Scope *Initial = S; >> - UnqualUsingDirectiveSet UDirs; >> + UnqualUsingDirectiveSet UDirs(*this); >> if (getLangOpts().CPlusPlus) { >> // Find the first namespace or translation-unit scope. >> while (S && !isNamespaceOrTranslationUnitScope(S)) >> >> Added: cfe/trunk/test/Modules/using-directive.cpp >> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Modules/ >> using-directive.cpp?rev=315402&view=auto >> ============================================================ >> ================== >> --- cfe/trunk/test/Modules/using-directive.cpp (added) >> +++ cfe/trunk/test/Modules/using-directive.cpp Tue Oct 10 18:19:11 2017 >> @@ -0,0 +1,62 @@ >> +// RUN: %clang_cc1 -fmodules -fmodules-local-submodule-visibility >> -fno-modules-error-recovery -fno-spell-checking -verify %s >> + >> +#pragma clang module build a >> +module a { explicit module b {} explicit module c {} } >> +#pragma clang module contents >> + >> +#pragma clang module begin a.b >> +namespace b { int n; } >> +#pragma clang module end >> + >> +#pragma clang module begin a.c >> +#pragma clang module import a.b >> +namespace c { using namespace b; } >> +#pragma clang module end >> + >> +#pragma clang module begin a >> +#pragma clang module import a.c >> +using namespace c; >> +#pragma clang module end >> + >> +#pragma clang module endbuild >> + >> +#pragma clang module import a.b >> +void use1() { >> + (void)n; // expected-error {{use of undeclared identifier}} >> + (void)::n; // expected-error {{no member named 'n' in the global >> namespace}} >> + (void)b::n; >> +} >> +namespace b { >> + void use1_in_b() { (void)n; } >> +} >> +namespace c { >> + void use1_in_c() { (void)n; } // expected-error {{use of undeclared >> identifier}} >> +} >> + >> +#pragma clang module import a.c >> +void use2() { >> + (void)n; // expected-error {{use of undeclared identifier}} >> + (void)::n; // expected-error {{no member named 'n' in the global >> namespace}} >> + (void)b::n; >> + (void)c::n; >> +} >> +namespace b { >> + void use2_in_b() { (void)n; } >> +} >> +namespace c { >> + void use2_in_c() { (void)n; } >> +} >> + >> +#pragma clang module import a >> +void use3() { >> + (void)n; >> + (void)::n; >> + (void)b::n; >> + (void)c::n; >> +} >> +namespace b { >> + void use3_in_b() { (void)n; } >> +} >> +namespace c { >> + void use3_in_c() { (void)n; } >> +} >> >> >> _______________________________________________ >> cfe-commits mailing list >> cfe-commits@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >> > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
