[cfe-dev] RFC: Name lookup overhaul

[Chris Lattner]

On Dec 11, 2008, at 7:49 AM, Douglas Gregor wrote:
>> This shrinks ScopedDecl by one pointer but grows DeclContext by ~3  
>> pointers, plus the memory that the vector keeps to hold the  
>> ScopedDecl's.  This is somewhat concerning because you also make  
>> every Record a DeclContext, and make FieldDecls into ScopedDecls.   
>> There are 1743 records in carbon.h and 1565 in Cocoa.h (according  
>> to "clang -print-stats INPUTS/Cocoa_h.m |& grep struct").  There  
>> are also thousands of enum decls that will get larger as well  
>> because they are already declcontext's (e.g. 2048 in cocoa).
>>
>> Going forward, is there any way to reduce this memory hit?
>
> Yes. Decls and LookupPtr should be moved out to a separate  
> structure, because declarations that aren't definitions (e.g.,  
> "struct X;") don't need the storage for information that's only  
> available in definitions. Fixing this is tied up with moving the  
> definition of entities out of the main declaration structures (e.g.,  
> RecordDecl shouldn't store the record's members; it should have a  
> pointer to a RecordDef, which stores the members).

Ok

> Doing that now would have made the patch even bigger :)

Sure, I'm mostly interested in the final point we'll arrive at, I'm  
all for incremental steps in the right direction :)

>> /// FieldDecl - An instance of this class is created by  
>> Sema::ActOnField to
>> /// represent a member of a struct/union/class.
>> -class FieldDecl : public NamedDecl {
>> +class FieldDecl : public ScopedDecl {
>> +  bool Mutable : 1;
>>
>> Do you forsee elimination of CXXFieldDecl completely forever?  If  
>> not, it would be nice to sink the Mutable bit down into the  
>> CXXFieldDecl class.  This would be a good place to put access  
>> control, mutable, and other C++ notions.
>
> Yes, I'm intent on killing off CXXFieldDecl. I was planning to kill  
> off CXXRecordDecl, too, but now I recall the discussion we had about  
> this a while back:
>
> 	http://lists.cs.uiuc.edu/pipermail/cfe-dev/2008-October/003199.html
>
> Perhaps we should discuss this sub-issue off-line and come back with  
> a policy for handling C/C++ distinctions in the ASTs.

Doug convinced me that having something like "BitFieldDecl" vs  
"FieldDecl" is more useful than having "CXXFieldDecl" vs "FieldDecl". :)

Both are fine with me, and we both agree that a "BitFieldDecl/ 
CXXFieldDecl" should be an internal implementation detail, the AST  
clients should just see FieldDecl, and the "give me the number of bits  
in the bitfield" method should be valid on it, but just return null if  
the field isn't a bitfield.

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

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

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

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

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

>> +++ lib/AST/Decl.cpp	(working copy)
>>
>> +  return new (Mem) FieldDecl(Decl::Field, DC, L, Id, T, BW,  
>> Mutable, PrevDecl);
>>
>> If CXXFieldDecl is gone for good, you can hardcode 'Decl::Field'  
>> into the FieldDecl ctor.
>
> There are other subclasses of FieldDecl in the Objective-C part.

Oh right, ok.

Thanks again Doug, this is great work,

-Chris