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

[Piotr Rak]

Hi Doug,

2009/2/5 Douglas Gregor <dgregor at apple.com>:
> On Feb 4, 2009, at 2:23 PM, Piotr Rak wrote:
>>
>> Attached refactors Sema::CppNameLookup in way, we do not reapeat
>> looking into same DeclContexts, also looks now it looks less chaotic
>> IMO :)
>> It also fixes MergeLookupResults. My recent patch caused it fail in
>> same cases, which is noticable with CppNameLookup change.
>
>
> This doesn't look right:
>
> +    if (HasFuncs) {
> +      // Filter out non-FunctionDecls from result set.
> +      for (DeclsSetTy::iterator FD=DI; FD!=DEnd; FD++)
> +        if (!isa<FunctionDecl>(*FD)) FoundDecls.erase(*FD);
> +      FoundLen = FoundDecls.size();
> +    }
>
> It's not that when we have functions we ignore everything else. Rather,
> object names (for functions, enumerators, variables, etc.) hide tag names
> (structs, enums, etc.), but only if those names themselves aren't ambiguous.
> I added this to the end of using-directive.cpp to test my theory:
>
> namespace Ni {
>  int i();
> }
>
> namespace NiTest {
>  using namespace A;
>  using namespace Ni;
>
>  int test() {
>    return i; // expected-error{{ambiguous}}
>  }
> }
>
> 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.

> 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.

Piotr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Refactor-CppLookupName.v2.patch
Type: text/x-patch
Size: 21039 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090205/bf2cb92b/attachment.bin>