[cfe-dev] Patch for IdentifierResolver

[Argiris Kirtzidis]

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

Chris Lattner wrote:
>>> What about "std::list<IdDeclInfo[512]>" (using a struct to hold the 
>>> array?)?  This saves an extra allocation of the list node, which is 
>>> not a big perf/memory win, but is tidier because you don't have to 
>>> delete[] the nodes.
>>
>> This is tidier indeed, but won't there be coping of the struct when 
>> it enters the container, thus making it a bit inefficient ?
> Chris Lattner wrote:
>
> 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.

>
> Some details:
>
>    // Register this decl in the current scope stack.
> -  New->setNext(Id->getFETokenInfo<ScopedDecl>());
> -  Id->setFETokenInfo(New);
> +  IdResolver.AddDecl(New, S);
>    S->AddDecl(New);
>
> This frequently occurs in Sema now.  How about adding a new method to 
> Sema, something like "PushOnScopeChains(New, S) which does the 
> IdResolver.adddecl, and the S->adddecl pair?

Done.

> +  /// Lookup - Find the non-shadowed decl that belongs to a particular
> +  /// Decl::IdentifierNamespace.
> +  NamedDecl *Lookup(const IdentifierInfo *II, unsigned NSI);
>
> The null and single identifier case are really common for this.  Would 
> it make sense to pull this code from lookup:
>
> +  if (!Ptr) return NULL;
> +
> +  if (isDeclPtr(Ptr)) {
> +    NamedDecl *D = static_cast<NamedDecl*>(Ptr);
> +    return (D->getIdentifierNamespace() == NS) ? D : NULL;
> +  }
>
> 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.

> +/// IdDeclInfo - Keeps track of information about decls associated to 
> a particular
> +/// identifier. IdDeclInfos are lazily constructed and assigned to an 
> identifier
> +/// the first time a decl with that identifier is shadowed in some 
> scope.
> +class IdDeclInfo {
>
> Please wrap this is an anonymous namespace and fit in 80 cols.

Done.

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

>
> 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.
I'm not sure about NextDeclarator, maybe adding a MultiDeclaratorDecl 
for those that need it to inherit from (using multiple inheritance like 
DeclContext) ?

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 ?