[cfe-dev] RFC: Name lookup overhaul

[Douglas Gregor]

On Dec 12, 2008, at 1:38 PM, Chris Lattner wrote:

> On Dec 11, 2008, at 7:49 AM, Douglas Gregor wrote:
>>
>>> +  enumerator_iterator enumerator_begin() const {
>>>
>>> Any reason to spell out "enumerator" instead of using enum_begin/ 
>>> enum_iterator/enum_end() etc?  I like the iterator interface a lot  
>>> better than the old getEnumConstantList stuff!
>>
>> "enum" could be "enumerator" or "enumeration". It's not a big  
>> deal---it's pretty obvious from the context that we're talking  
>> about enumerators here---but I'm not a fan of ambiguous shorthand.
>
> Shorthand can be ok as long as it is consistent.  How about enum for  
> enumerator and enumcst for enumerator constant?  Am I missing  
> something?

enumerator == enumerator constant, but the latter isn't a standard  
term. If we were being picky, EnumDecl should be EnumerationDecl (it  
declares an enumeration type) and EnumConstantDecl should be  
EnumeratorDecl (it declares an enumerator, e.g., one of the values in  
the enumeration type). That's why I don't like "enum"... it's  
shorthand for both of the standard terms.

>
>>> +  class field_iterator {
>>>
>>> There is a lot of commonality between field_iterator and  
>>> enumerator_iterator, maybe they should be genericized?
>>
>> field_iterator has a bit of logic to skip over declarations that  
>> aren't fields. We could certainly have an iterator adaptor that  
>> effectively performs a cast<T> on the underlying iterator.
>
> This can be handled by making the template take a functor that  
> decides whether an entry should be returned or skipped.  The default  
> functor (used by enums) would always return "don't skip".

The proper generic factoring is to have an iterator adaptor that does  
the cast<T> (a "transform" iterator, in Boost parlance) and another  
that does the skipping based on a function object (a "filter"  
iterator, in Boost parlance):

	http://www.boost.org/doc/libs/1_37_0/libs/iterator/doc/filter_iterator.html
	http://www.boost.org/doc/libs/1_37_0/libs/iterator/doc/transform_iterator.html

Right now, we only have one iterator that does the skipping  
(field_iterator), which also does a dyn_cast rather than a cast, so  
it's not terribly high priority to factor that out. We could certainly  
build a simple version of the transform iterator and use that for the  
casting iterators.

>>> +  typedef field_iterator field_const_iterator;
>>>
>>> Naughty naughty :)
>>
>> Busted!
>
> incidentally, this is something else that is useful to genericize  
> on, instead of duplicating the iterator class.  That way,  
> field_iterator is just a typedef for a some_iterator<FieldDecl,...  
> and const_iterator is some_iterator<const FieldDecl, ...

This could certainly be a part of the cast<>'ing transform iterator.

>>> +  /// Entity - The entity with which this scope is associated. For
>>> +  /// example, the entity of a class scope is the class itself, the
>>> +  /// entity of a function scope is a function, etc. This field is
>>> +  /// maintained by the Action implementation.
>>> +  void *Entity;
>>>
>>> This is orthogonal to this patch, but I wonder if Scope::Entity  
>>> could be used to simplify your recent "if(int x = ..)" patch.  If  
>>> the entity for the if-scope was the if statement itself, you  
>>> wouldn't need an "is defined in an if" bit on decl.
>>
>> We can detect the "if" part pretty easy, by looking up the scope  
>> chain to the control-scope parent. It's knowing that we're in the  
>> "else" block that required the extra bits... perhaps there's still  
>> a better way. I'll think about it.
>
> Ahhh, good point.  One somewhat hackish solution would be to only  
> set the entity when parsing the else block instead of setting it  
> when the scope is formed.

It feels so much cleaner to associate the entity with the "if" itself.

>
>>> +    // Put this declaration into the appropriate slot.
>>> +    TwoNamedDecls Val;
>>> +    Val.Decls[IndexOfD] = D;
>>> +    Val.Decls[!IndexOfD] = 0;
>>
>> Blech. Using '!' for numerical values is so... hackish, no?
>
> *shrug*, it's a hard call, and I don't have a strong opinion.  If  
> you like it the other way, I'm ok with that too.

I went with your solution.

	- Doug