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