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

[Piotr Rak]

Hi Doug,

2009/2/5 Douglas Gregor <dgregor at apple.com>:
> Hi Piotr,
>
> On Feb 4, 2009, at 7:44 PM, Piotr Rak wrote:
>>
>> 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.

I'll do that, but we will have to be extra careful to avoid leaks anyway.
Could we just add destructor only for debug mode, something like:

#ifndef NDEBUG
LookupResult::~LookupResult() {
  assert(<did_cleanup>);
}
#endif

> +
> +  // 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?

Indeed, I added this to my local copy (where I have basicly
implemented [namespace.qual]).
Unfortunately Sema::ActOnFunctionDeclarator() does pretty wired stuff,
things like:

namespace A {}

void A::f() {}

compiles silently with trunk, and few other funny things..., so I must
first straighten this out. I think I already know how to fix it.

Same bug, makes this fail:

namespace X {
  void foo();
}

namespace Y {
  using namespace X;
}

void Y::foo() {} //expected-error{{no member}}

I've attached my changes, but those are not ready to commit.

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

Right, I fail again.

> 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();
>

Do we want 'clang -verify' warn when such things happen? Now in such
case tests will silently passes.

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

I will look into it.

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

True, it derefences already invalid iterator.

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

Great! Thanks for review and fixing my failures, again.

> The commit is here:
>
>
>  http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20090202/011825.html
>
>        - Doug
>

Piotr
-------------- next part --------------
A non-text attachment was scrubbed...
Name: namespace.qual.patch
Type: text/x-patch
Size: 11788 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20090206/57129fc4/attachment.bin>