r315402 - [modules] Only take visible using-directives into account during name lookup.

Richard Smith via cfe-commits cfe-commits at lists.llvm.org
Mon Oct 30 15:40:12 PDT 2017


On 30 October 2017 at 15:12, Vassil Vassilev via cfe-commits <
cfe-commits at 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 at lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20171030/d6f89b11/attachment-0001.html>


More information about the cfe-commits mailing list