[cfe-dev] Patch for IdentifierResolver

[Chris Lattner]

On Apr 11, 2008, at 6:52 PM, Argiris Kirtzidis wrote:

> Implemented suggestions and commited:
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005145.html
> http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005147.html

Nice!

>> Depends on how it is declared.  If declared as an actual array,  
>> yes, each element will be copy constructed.  If you declare it as a  
>> SmallVector, then only the elements that are used (i.e., none of  
>> them :) will be copied.  This also defers initialization of  
>> elements until they are used.
>
> Good idea, I used a std::vector (SmallVector seems inappropriate  
> since lots of elements are allocated) and there was a tiny bit of  
> efficiency boost too.

Ok.  The only significant difference between SmallVector and vector is  
that smallvector has space for "n" nodes in itself, so it doesn't  
require a malloc on the side.  This is not a big deal in this case :)

>> into an inline method in the header?  If this fails, it could call  
>> the out-of-line version.  If this is a speedup, it might be  
>> worthwhile to do for AddDecl and RemoveDecl as well.
>
> I didn't quite see much of a difference by inlining it. I can send  
> you a patch to try it out yourself if you want.

I believe you, thanks for investigating.

>> +/// IdDeclInfoMap - Associates IdDeclInfos with Identifiers.
>> +/// Allocates 'pools' (arrays of IdDeclInfos) to avoid allocating  
>> each
>> +/// individual IdDeclInfo to heap.
>> +class IdentifierResolver::IdDeclInfoMap {
>>
>> This doesn't need to be a nested class anymore, it can just be a  
>> top-level class in an anon namespace.  Doing so requires defining  
>> the IdDeclInfos member of IdentifierResolver as a void* though.
>
> Is it neccessary to put in the anonymous namespace ? I find it a bit  
> uncomfortable to use a void* member for IdentifierResolver..

Putting it in an anonymous namespace is tidy, but the big win is that  
the methods become 'static' to the translation unit instead of being  
externally visible.  This reduces the number of symbols exported by  
the .o file, reducing load on the static and dynamic linker.  It also  
gives more information to the optimizer.

It's not *that* big of a deal of course, but it's a small win.

>>
>> Finally, I think this permits some cleanups of the AST.  Can the  
>> ScopeDecl class completely disappear?

>>
> ScopedDecl uses NextDeclarator and Next members. Next can be removed  
> or it could be used to iterate over decls that are contained inside  
> a DeclContext.

Right.

>
> I'm not sure about NextDeclarator, maybe adding a  
> MultiDeclaratorDecl for those that need it to inherit from (using  
> multiple inheritance like DeclContext) ?

Does anything currently use this information?  I'm find with losing  
the declarator->definition mapping if there is nothing using it right  
now.

> Also, should I add a TranslationUnitDecl (or some other name) just  
> to server as DeclContext ? There's already the TranslationUnit class  
> and maybe it should be
> modified in some way to provide DeclContext along with its other  
> functions, but at the moment is it reasonable to add a separate  
> class for a T.U. DeclContext ?

That would be great.  I think a top level TranslationUnitDecl is a  
nice unifying concept.  Moving the existing stuff in TranslationUnit  
makes perfect sense to me.

-Chris