[cfe-dev] [Patch] Refactor Sema::CppLookupName

[Douglas Gregor]

Hi Piotr,

On Feb 4, 2009, at 7:44 PM, Piotr Rak wrote:
> 2009/2/5 Douglas Gregor <dgregor at apple.com>:
>>
>> and, rather than producing an ambiguity, it complains that it can't  
>> return
>> an int(void) from a function whose return type is "int". I suspect  
>> that A::i
>> is getting thrown out by the loop mentioned above.
>
> Indeed, thanks for pointing that out! Attached should fix that.
>
>> I really think that FoundOverloaded, and the optimization it's  
>> attempting to
>> do, is overly complicating MergeLookupResults. I suggest that the
>> FoundOverloaded optimization be removed for now, until we have the  
>> semantics
>> solidly implemented. Every Decl that we see (either due to  
>> LResult::Found or
>> LResult::FoundOverloaded) gets dumped into FoundDecls. For  
>> TagDecls, get the
>> canonical tag decl before putting the decl into FoundDecls.
>
> It was not optimization any longer, since we started to have access
> Decls by LookupResult::begin/end without, but I failed to notice that.
> This version yields one OverloadFunctionDecl allocation too.
>
> Current approach (LookupResult::OverloadedDeclSingleDecl), still has
> one flaw, unfortunately consumers of LookupResult using new interface
> (begin/end) to access overloads, need to check if we store
> OverloadedFunctionDecl or iterators, to free memory. I don't see any
> obvious solution for that now, I will keep that in mind.

Okay. We can add a Destroy method that knows how to deallocate  
LookupResults data structures. That would also be useful to deallocate  
base paths and such.

+
+  // No name lookup results, return early.
+  if (I == End) return LResult::CreateLookupResult(Context, 0);
+

We could also return early if there's only one lookup result, right?

>
>> At the end of the main loop in MergeLookupResults, FoundDecls will  
>> contain
>> all of the non-unique decls. We can go through those keeping track  
>> of what
>> kinds of names we see: object names, function names, and tag names.  
>> Then,
>> determine whether they conflict or one hides the other, generating an
>> ambiguous lookup if they conflict and hiding the tag names if they  
>> don't
>> conflict.
>
> Yes, this is much cleaner way. I hope, I got it right this time.

It's looking a lot better! I've committed it with some tweaks of my  
own, which I wanted to comment on:

+  if (FoundDecls.size() == 1) {
+    // 1) Exactly one result.
+  } else if (FoundDecls.size() - TagNames == 1) {
+    // 2) Ordinary name hides tag.
+    NameHidesTags = true;

This means that an unambiguous object name (or set of function names)  
will hide tag names, even if the tag names result in an ambiguity.  
However, we need to report an ambiguity in this case. I've tweaked the  
code by adding an "else if (TagNames > 1) Ambiguous = true;" between  
cases 1 and 2, along with this test:

namespace OneTag {
   struct X; // expected-note{{candidate found by name lookup is  
'OneTag::X'}}
}

namespace OneFunction {
   void X(); // expected-note{{candidate found by name lookup is  
'OneFunction::X'}}
}

namespace TwoTag {
   struct X; // expected-note{{candidate found by name lookup is  
'TwoTag::X'}}
}

namespace FuncHidesTagAmbiguity {
   using namespace OneTag;
   using namespace OneFunction;
   using namespace TwoTag;

   void test() {
     (void)X(); // expected-error{{reference to 'X' is ambiguous}}
   }
}

Interesting, this test uncovered an unfortunate issue. Since we added  
code to report ambiguous type-name lookups in Sema::getTypeName, we  
actually get *two* sets of ambiguity diagnostics regarding

	X();

The first diagnostic comes when we check whether 'X' is a type name  
(using Sema::getTypeName, to see if this is a C++ functional cast),  
and then later we check whether we can build an expression for  
'X' (using Sema::ActOnIdentifierExpr), and both of those issue the  
diagnostic. Ideas welcome!

Moving along with the patch...

+    if (NameHidesTags) {
+      // 2, 3): Remove tag-names hidden by ordinary-names.
+      DeclsSetTy::iterator DI = FoundDecls.begin(), DEnd =  
FoundDecls.end();
+      for (DI = FoundDecls.begin(); (DI != DEnd) && (TagNames > 0); + 
+DI) {
+        if (isa<TagDecl>(*DI)) {
+          FoundDecls.erase(*DI);
+          --TagNames;
+        }
+      }
+      assert((TagNames == 0) && "Failed remove all TagDecls?");

It's almost never safe to erase something from a container when  
looping over the container, and SmallPtrSet doesn't even provide an  
interface to make this safe. That said, since NameHidesTags should  
only be true when there is a single tag name left, so in the version I  
committed we keep track of that one tag decl and remove it from  
FoundDecls. No more loop!

The commit is here:

	http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090202/011825.html

	- Doug