[cfe-dev] Qualified and unqualified name lookup and IdResolver

[Chris Lattner]

On Dec 10, 2008, at 10:49 AM, Douglas Gregor wrote:

>>
>> +  lookup_result lookup(ASTContext &Context, DeclarationName Name);
>> +  lookup_const_result lookup(ASTContext &Context, DeclarationName  
>> Name) const;
>>
>> Since the impl of the second lookup function is just a  
>> const_cast'ing forwarding function for the first one, it might be  
>> worth putting it inline in the header.
>
> Then we'd need to include "clang/AST/DeclarationName.h".

Ok.  That's a good reason to leave it out of line if it isn't already  
implicitly included.

>> +  if (ScopedDecl *SD = dyn_cast<ScopedDecl>(D))
>> +    CurContext->addDecl(Context, SD, SupportQualifiedNameLookup);
>> +  else if (CXXFieldDecl *FD = dyn_cast<CXXFieldDecl>(D))
>> +    FD->getParent()->insert(Context, FD);
>> +
>>  IdResolver.AddDecl(D);
>> }
>>
>> Is there an "else" for this?  If not, it would be good to make it  
>> assert and die.  If so, it would be good to add a comment.
>
> There's no "else", so I've added a comment. Basically, if it's going  
> to be in a DeclContext, it's going to be a ScopedDecl. No  
> exceptions. (For reference: CXXFieldDecl is gone in the new patch,  
> and FieldDecl is a ScopedDecl).

Ok, a comment is good, an assert (if possible) is better :)

>> +	// Check whether the IdResolver has anything in this scope.
>> +	// FIXME: The isDeclScope check could be expensive. Can we do  
>> better?
>> +	while (I != IdResolver.end() && S->isDeclScope(*I)) {
>> +	  if ((*I)->getIdentifierNamespace() & NS)
>> +	    return *I;
>> +	
>> +	  ++I;
>> +	}
>>
>> How about getting the decls context and using that?  Do scopes have  
>> an associated context?
>
> Some scopes have an associated "entity", which ends up being a  
> DeclContext, but there's no mapping from DeclContext -> Scope  
> (because Scope is a transient thing only available at parse time).  
> The isDeclScope check is just a lookup in a (typically relatively  
> small) set, so it shouldn't be *that* expensive. I'll leave the  
> FIXME and think about it more, later :)

Sure, sounds good.

>> +	// If there is an entity associated with this scope, it's a
>> +	// DeclContext. We might need to perform qualified lookup into
>> +	// it.
>> +	DeclContext *Ctx = static_cast<DeclContext *>(S->getEntity());
>> +	while (Ctx && Ctx->isFunctionOrMethod())
>> +	  Ctx = Ctx->getParent();
>>
>> Can you have more than one function/method context?  If no, please  
>> convert the while to if.
>
> IIUC,  we could have a block context inside of a function context.

Does a block context satisfy the predicate "isFunctionOrMethod"?  If  
so, we need to rename isFunctionOrMethod :)

or do you mean !Ctx->isFunctionOrMethod() ?

>> For example, could 'enableLazyBuiltinCreation' be handled as a  
>> "lookup" on the global (or maybe a new 'builtin') scope instead of  
>> as a special case hack?
>
> Well, maybe. It depends somewhat on whether we want to permit  
> qualified lookup to find these names ('::__builtin_foo') or not. One  
> could imagine that they live in an imaginary scope outside of the  
> translation unit, so you can never actually find them via  
> unqualified lookup.

I don't know what GCC does with this, but I would assume they are only  
ever found

>> Another option would be to pull the array/map code into its own  
>> generic class (out of the DeclContext class), which might be the  
>> real best solution.
>
> What it does is far too specific to end up in its own generic class.

Ok, you convinced me on irc.

>> Finally, when the dust settles on name lookup, can you please add  
>> an overview to the Internals doc? :)  The issues are nuanced enough  
>> that a high level overview would be very useful.
>
> Yeah, I will. I want to make sure I've gotten it right for the truly  
> ugly cases (using declarations and such) before investing too much  
> time in documentation.

Thanks Doug!  I'll take a look at your new patch tomorrow, but feel  
free to commit it if I don't get to it in time.

-Chris