<html><head><base href="cid:F34AC343-545C-451C-BD1D-AD69724B8FBE@apple.com"><base href="cid:F34AC343-545C-451C-BD1D-AD69724B8FBE@apple.com"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><base href="cid:F34AC343-545C-451C-BD1D-AD69724B8FBE@apple.com">Hi Piotr,<div><br><div><div>On Feb 2, 2009, at 3:53 PM, Piotr Rak wrote:</div><blockquote type="cite"><div>Thanks for review, and great comments!<br><br>I have addressed most of issues in attached version.<br>It also partially fixes other thing I have noticed, similar failure to<br>one from first version of patch with namespaces, now with tags.<br>Fix is not complete though, but behaviour is right. One commented out<br>in test is incorrectly diagnosed. I'll try send fix ASAP.<br></div></blockquote><div><br></div><div>Got it; some review and comments follow, but I've committed your patch already with a few tweaks. Thank you!</div><div><br></div><blockquote type="cite"><div>There is risk we don't check LookupResult for ambiguities in more<br>cases too... We need will need more tests. </div></blockquote><div><br></div><div>Yes, definitely. Eventually, it's probably safest if we remove the implicit conversion from LookupResult -> Decl*, then audit all uses of getAsDecl. However, there are other issues we need to solve first :)</div><br><blockquote type="cite"><div>I have checked with gcc<br>testsuite, and we pass all tests that have 'using namespace', and<br>don't include standard library headers, use using-declaration, or<br>require strong-using now.<br></div></blockquote><div><br></div><div>Wonderful.</div><br><blockquote type="cite"><div><blockquote type="cite">This is what I was looking for. We can cache the results of this operation<br></blockquote><blockquote type="cite">in the Scope structure or (in some cases) DeclContext. Performing this<br></blockquote><blockquote type="cite">operation once per LookupName call would be quite painful; but if the number<br></blockquote><blockquote type="cite">of times we need to do this is proportional instead to the number of<br></blockquote><blockquote type="cite">using-directives, that would be fine.<br></blockquote><blockquote type="cite"><br></blockquote><br>I would like to avoid making DeclContext heavier, without big<br>benfit... Scope sounds better to me.I have put some effort to avoid<br>that, because:<br><br>- C doesn't need it.</div></blockquote><blockquote type="cite"><div>- There will be houndreds DeclContext's in typical code<br>- I will be surprised if there we were many more than 20 Scope objects<br>in typical<br> code at one time<br>- Scope has short pretty life, if we compare it to DeclContext.<br>- Other AST clients can't benefit from that information anyway...<br></div></blockquote><div><br></div><div>Sure, but qualified name lookup goes through DeclContexts, not through Scopes, so qualified name lookup can't benefit from caching if it isn't cached in the DeclContext.</div><div><br></div><div>Anyway, if we generalized the DenseMap inside DeclContext so that it was relatively easy to add "extra" names (like you did with the special DeclarationName for using directives), then it should be easy to create a special name for the cached set of namespaces and put those into the DeclContext. That can come with a later patch, of course.</div><br><blockquote type="cite"><div><blockquote type="cite">+ } else if (UsingDirectiveDecl *UD = dyn_cast<UsingDirectiveDecl>(D)) {<br></blockquote><blockquote type="cite">+ // print using-directive decl (e.g. "using namespace x;")<br></blockquote><blockquote type="cite">+ const char *ns;<br></blockquote><blockquote type="cite">+ if (const IdentifierInfo *II = UD->getUsedNamespace()->getIdentifier())<br></blockquote><blockquote type="cite">+ ns = II->getName();<br></blockquote><blockquote type="cite">+ else<br></blockquote><blockquote type="cite">+ ns = "<anonymous>";<br></blockquote><blockquote type="cite">+ fprintf(F, "\"%s %s;\"",UD->getDeclKindName(), ns);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Is it even possible to have a using directive that nominates an anonymous<br></blockquote><blockquote type="cite">namespace? I guess it depends on how we implement unnamed namespaces.<br></blockquote><blockquote type="cite"><br></blockquote><br>That reveals, how I plan deal with anonymous namespaces for name lookup :)<br>I wanted add implicit using-directive in enclosing context (as<br>Sebastian already pointed out). This should be enough, assuming that,<br>we work only with single translation unit.<br></div></blockquote><div><br></div><div>That's a perfectly reasonable assumption.</div><br><blockquote type="cite"><div>In fact, I was planing to implement unqualified/qualified name lookup,<br>gcc strong-using/inline namespace and anonymous namespaces together,<br>but later it turned out to be too big for one patch!<br></div></blockquote><div><br></div><div>Yikes! It's good to take those separately... anonymous namespaces should be a simple follow-on to this patch. Inline namespaces, on the other hand, don't need to interact with this patch: they should just be transparent DeclContexts (see the comment in DeclContext::isTransparentContext() in lib/AST/DeclBase.cpp). </div><br><blockquote type="cite"><div><blockquote type="cite">Should Sema::ActOnUsingDirective assert that the scope S it gets is a<br></blockquote><blockquote type="cite">DeclScope?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ // Ensure, that decls are being added to orginal namespace.<br></blockquote><blockquote type="cite">+ // Ctx = Ctx->getPrimaryContext(); XXX: needless<br></blockquote><blockquote type="cite">+ Ctx->addDecl(UDir);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Right, we don't need that Ctx = Ctx->getPrimaryContext() here, but we will<br></blockquote><blockquote type="cite">need it later...<br></blockquote><br>No we won't, using-directive should be added to lexical context, and<br>only be visible<br>in semantic context, just like other NamedDecls. This is how<br>DeclContext::addDecl works now, so we don't need anything special<br>here, right?<font class="Apple-style-span" color="#006312"><br></font></div></blockquote><div><br></div><div>Right. I see that the getPrimaryContext() is already implicit in the way we deal with nominated namespaces, so my comment about needing it later was wrong.</div><div><br></div><blockquote type="cite"><div><font class="Apple-style-span" color="#006312"><br></font><blockquote type="cite">+ case LResult::AmbiguousBaseSubobjectTypes:<br></blockquote><blockquote type="cite">+ case LResult::AmbiguousBaseSubobjects: {<br></blockquote><blockquote type="cite">+ // FIXME: Do we care about later possibly found<br></blockquote><blockquote type="cite">+ // AmbiguousBaseSubobjectTypes, or AmbiguousBaseSubobjects?<br></blockquote><blockquote type="cite">+ // Is it ever possible?<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I don't believe this is ever possible, since using directives can't occur at<br></blockquote><blockquote type="cite">class scope. We should probably have an assert() here.<br></blockquote><br>It shouldn't be possible, however it is, probably because we look more<br>than once in same DeclContext.<br></div></blockquote><div><br></div><div>Sure, but we never look into a DeclContext that's associated with a class, because using directives don't ever nominate classes.</div><br><blockquote type="cite"><div><blockquote type="cite">+ case LResult::FoundOverloaded:<br></blockquote><blockquote type="cite">+ if (FoundOverloaded) {<br></blockquote><blockquote type="cite">+ // We have one spare OverloadedFunctionDecl already, so we store<br></blockquote><blockquote type="cite">+ // its function decls and free it.<br></blockquote><blockquote type="cite">+ OverloadedFunctionDecl *Ovl =<br></blockquote><blockquote type="cite">+ cast<OverloadedFunctionDecl>(I->getAsDecl());<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ OverloadedFunctionDecl::function_iterator<br></blockquote><blockquote type="cite">+ FI = Ovl->function_begin(),<br></blockquote><blockquote type="cite">+ FEnd = Ovl->function_end();<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ for (FI; FI != FEnd; ++FI)<br></blockquote><blockquote type="cite">+ FoundDecls.push_back(*FI);<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ Ovl->Destroy(Context);<br></blockquote><blockquote type="cite">+ } else {<br></blockquote><blockquote type="cite">+ // First time we found OverloadedFunctionDecl, we want to conserve<br></blockquote><blockquote type="cite">+ // it, and possibly add other found Decls later.<br></blockquote><blockquote type="cite">+ FoundOverloaded = cast<OverloadedFunctionDecl>(I->getAsDecl());<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite">+ break;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">In the "else" branch, don't you need to add all of the decls to FoundDecls?<br></blockquote><br>No, because I reuse first OverloadedFunctionDecl I find, so it has<br>them already, do you see otherwise?<br></div></blockquote><div><br></div><div>Okay, I understand now. The FoundDecls not in the OverloadedFunctionDecl get added to the OverloadedFunctionDecl later.</div><br><blockquote type="cite"><div><blockquote type="cite">+ DeclsVecTy::iterator DI = FoundDecls.begin(), DEnd = FoundDecls.end();<br></blockquote><blockquote type="cite">+ std::sort(DI, DEnd);<br></blockquote><blockquote type="cite">+ DEnd = std::unique(DI, DEnd);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I have to wonder if it would be easier just to insert everything into a<br></blockquote><blockquote type="cite">SmallPtrSet and then just iterate over the results, especially given the<br></blockquote><blockquote type="cite">code below it:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ if (FoundOverloaded) {<br></blockquote><blockquote type="cite">+ // We found overloaded functions result. We want to add any other<br></blockquote><blockquote type="cite">+ // found decls, that are not already in FoundOverloaded, and are<br></blockquote><blockquote type="cite">functions<br></blockquote><blockquote type="cite">+ // or methods.<br></blockquote><blockquote type="cite">+ OverloadedFunctionDecl::function_iterator<br></blockquote><blockquote type="cite">+ FBegin = FoundOverloaded->function_begin(),<br></blockquote><blockquote type="cite">+ FEnd = FoundOverloaded->function_end();<br></blockquote><blockquote type="cite">+ for (DI ; DI < DEnd; ++DI) {<br></blockquote><blockquote type="cite">+ FunctionDecl *Fun = dyn_cast<FunctionDecl>(*DI);<br></blockquote><blockquote type="cite">+ if (Fun && (std::find(FBegin, FEnd, Fun) != FEnd)) { /* Skip.*/ }<br></blockquote><blockquote type="cite">+ else DEnd = std::remove(DI, DEnd, *DI);<br></blockquote><blockquote type="cite">+ }<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This is O(n^2) in [DI, DEnd), because remove() is linear time. Sure, [DI,<br></blockquote><blockquote type="cite">DIEnd) should be pretty small---and so should [FBegin, FEnd) which we search<br></blockquote><blockquote type="cite">through linearly---but I think this whole section would be cleaner (and no<br></blockquote><blockquote type="cite">less efficient) with SmallPtrSet.<br></blockquote><br>Would it fair, to fix it once OverloadedFunctionDecl dies, from<br>exactly small detail you mentioned below... Decls need to be added in<br>same order we found it.<br></div></blockquote><div><br></div><div>We can fix it later, although I don't think we need this fix to be tied to the demise of OverloadedFunctionDecl.</div><br><blockquote type="cite"><div><blockquote type="cite">+ // We might find FunctionDecls in two (or more) distinct DeclContexts.<br></blockquote><blockquote type="cite">+ Decl *D = MaybeConstructOverloadSet(Context, DI, DEnd);<br></blockquote><blockquote type="cite">+ if ((FoundLen == 1) || isa<OverloadedFunctionDecl>(D))<br></blockquote><blockquote type="cite">+ return LResult::CreateLookupResult(Context, D);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">It's very interesting that this implements 3.4.3.2p5 (a comment and quote<br></blockquote><blockquote type="cite">from the standard here would help!), although I think there's one issue:<br></blockquote><blockquote type="cite">MaybeConstructOverloadSet(Context, DI, DEnd) expects that, if the sequence<br></blockquote><blockquote type="cite">[DI, DEnd) contains both object and tag names, that the object names precede<br></blockquote><blockquote type="cite">the tag names. This won't necessarily be the case with [DI, DEnd), since we<br></blockquote><blockquote type="cite">put decls into FoundDecls in the order we found them.<br></blockquote><br>Are you sure it is right, in last final draft I have 3.4.3.2.p5 is:<br><br>"During the lookup of a qualified namespace member name, if the lookup<br>finds more than one declaration of the member, and if one<br>declaration introduces a class name or enumeration name and the other<br>declarations either introduce the same object, the same enumerator or<br>a set of functions, the non-type name hides the class or enumeration<br>name if and only if the declarations are from the same namespace;<br>otherwise<br>(the declarations are from different namespaces), the program is ill-formed."<br></div></blockquote><div><br></div><div>Oh, interesting! I had misread that.</div><div><br></div><blockquote type="cite"><div><blockquote type="cite">+ // Collect UsingDirectiveDecls in all scopes, and recursivly all<br></blockquote><blockquote type="cite">+ // nominated namespaces by those using-directives.<br></blockquote><blockquote type="cite">+ // UsingDirectives are pushed to heap, in common ancestor pointer<br></blockquote><blockquote type="cite">+ // value order.<br></blockquote><blockquote type="cite">+ UsingDirectivesTy UDirs;<br></blockquote><blockquote type="cite">+ for (Scope *SC = Initial; SC; SC = SC->getParent())<br></blockquote><blockquote type="cite">+ AddScopeUsingDirectives(SC, UDirs);<br></blockquote><blockquote type="cite">+<br></blockquote><blockquote type="cite">+ // Sort heapified UsingDirectiveDecls.<br></blockquote><blockquote type="cite">+ std::sort_heap(UDirs.begin(), UDirs.end());<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Could we move this computation down below the part that looks in local<br></blockquote><blockquote type="cite">scopes for names? For example, if we have something like this:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> namespace N { int i; }<br></blockquote><blockquote type="cite"> using namespace N;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> int f(int i) {<br></blockquote><blockquote type="cite"> return i;<br></blockquote><blockquote type="cite"> }<br></blockquote><br>Now 'i' is found before sort for this case, we return result in line 562:<br> return std::make_pair(true, Result);<br><br>I have checked that in debugger, do you see otherwise?<br></div></blockquote><div><br></div><div>I see it now. Thanks.</div><br><blockquote type="cite"><div><blockquote type="cite">When doing name lookup for "i", we'll never look into a namespace or global<br></blockquote><blockquote type="cite">scope, so there's no reason to go through the effort of looking for using<br></blockquote><blockquote type="cite">directives.<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">+ for (; S && isFunctionScope(S); S = S->getParent()) {<br></blockquote><blockquote type="cite">// ...<br></blockquote><blockquote type="cite">+ // NB: Icky, we need to look in function scope, but we need to check<br></blockquote><blockquote type="cite">its<br></blockquote><blockquote type="cite">+ // parent DeclContext, in case it is out of line method definition.<br></blockquote><blockquote type="cite">+ if (S->getEntity() && isFunctionScope(S))<br></blockquote><blockquote type="cite">+ break;<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">This doesn't look right... function scopes have entities (the FunctionDecl);<br></blockquote><blockquote type="cite">it's only when that entity is a C++ member function that we need to look<br></blockquote><blockquote type="cite">into its (semantic) parent *after* we've looked in function scope. This look<br></blockquote><blockquote type="cite">should exit out once we hit a scope S whose entity is a NamespaceDecl or<br></blockquote><blockquote type="cite">TranslationUnitDecl; after that, we need to start worrying about using<br></blockquote><blockquote type="cite">directives.<br></blockquote><br>If we did not break here; Scope->getEntity() would be translation-unit<br>or namespace, for out of line member function definition, and we would<br>not perform member name lookup. It is true, we could do<br>LookupQualifiedName here, and stop later at translation-unit namespace<br>Scope. Is that what you wanted me to implement?<br></div></blockquote><div><br></div><div>I think so :)</div><div><br></div><div>I'm hoping we can leverage LookupQualifiedName when we're looking into scopes that also permit qualified name lookup (classes, namespaces, translation unit/global scope). However, your comment below is making me think that we shouldn't try to do this.</div><br><blockquote type="cite"><div><blockquote type="cite">+ for (llvm::tie(UI, UEnd) =<br></blockquote><blockquote type="cite">+ FindUsingDirsByAncestors(UDirs, Ctx); UI != UEnd; ++UI) {<br></blockquote><blockquote type="cite">+ // FIXME: We will have to ensure, that we won't consider<br></blockquote><blockquote type="cite">+ // again using-directives during qualified name lookup!<br></blockquote><blockquote type="cite">+ // (Once using-directives support for qualified name lookup gets<br></blockquote><blockquote type="cite">+ // implemented).<br></blockquote><blockquote type="cite">+ if (LookupResult R = LookupQualifiedName((*UI)->getUsedNamespace(),<br></blockquote><blockquote type="cite">+ Name, NameKind, RedeclarationOnly))<br></blockquote><blockquote type="cite">+ LookupResults.push_back(R);<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Regarding this FIXME, I think CppLookupName will become a bit easier if<br></blockquote><blockquote type="cite">LookupQualifiedName understood how to search for those names within its<br></blockquote><blockquote type="cite">DeclContext that should be found via using directives. In particular,<br></blockquote><blockquote type="cite">LookupQualifiedName could take an optional "UsingDirectivesTy const *UDirs"<br></blockquote><blockquote type="cite">parameter that it would use to determine which other namespaces to search<br></blockquote><blockquote type="cite">in. I think the end result would be clearer (and more functional).<br></blockquote><br>LookupQualifiedName will need to get logic for using-directives, which<br>is quite different from what we want for unqualified name lookup. That<br>means, it would get 2 modes for:<br> - unqualified name lookup<br> - qualified name lookup<br><br>Qualified name lookup is quite complicated too unfortunately, involves<br>tracking visited<br>contexts, etc...<br>Beside we will probably need off switch for considering<br>using-directives for ADL, because of 3.4.2.p3.</div></blockquote><blockquote type="cite"><div>If it is not essential to land this patch, I would like to implement<br>qualified name lookup first.<br></div></blockquote></div><br></div><div>Okay, understood. </div><div><br></div><div>I'm planning to work on argument dependent lookup now (it's next on "the list"). I don't think it will collide with what you're doing because it's a narrow form of lookup that is not likely to go through Lookup*Name at all. We'll see :)</div><div><br></div><div>One thing I noticed about ActOnTag:</div><div><br></div><div><div>@@ -2824,8 +2826,20 @@ Sema::DeclTy *Sema::ActOnTag(Scope *S, unsigned TagSpec, TagKind TK,</div><div> } else if (Name) {</div><div> // If this is a named struct, check to see if there was a previous forward</div><div> // declaration or definition.</div><div>- Decl *D = LookupName(S, Name, LookupTagName);</div><div>- PrevDecl = dyn_cast_or_null<NamedDecl>(D);</div><div>+ LookupResult R = LookupName(S, Name, LookupTagName);</div><div>+ if (R.isAmbiguous()) {</div><div>+ DiagnoseAmbiguousLookup(R, Name, NameLoc);</div><div>+ // FIXME: This is not best way to recover from case like:</div><div>+ //</div><div>+ // struct S s;</div><div>+ //</div><div>+ // causes needless err_ovl_no_viable_function_in_init latter.</div><div>+ Name = 0;</div><div>+ PrevDecl = 0;</div><div>+ Invalid = true;</div><div>+ }</div><div>+ else</div><div>+ PrevDecl = dyn_cast_or_null<NamedDecl>(static_cast<Decl*>(R));</div><div> </div><div> if (!getLangOptions().CPlusPlus && TK != TK_Reference) {</div><div> // FIXME: This makes sure that we ignore the contexts associated</div><div><br></div><div>This is going to diagnose ambiguities even when we shouldn't because, for example, we might be declaring a class or enum type in a new scope, but LookupName is finding something ambiguous in a different (parent) scope. I added two test cases here, one involving using-directives and one involving base-class lookup, that trigger the bug.</div><div><br></div><div>However, I think it's best if I tackle this one. I've been doing a bit of work in ActOnTag recently, and I have both the C and C++ rules fresh in my mind.</div><div><br></div><div>Thanks for the great contribution!</div><div><br></div></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug<br></div></body></html>