[cfe-dev] Patch for IdentifierResolver

[Chris Lattner]

On Apr 9, 2008, at 11:51 PM, Argiris Kirtzidis wrote:
> Attached new patch with the suggestions.

Excellent!  I've committed it here:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005124.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20080407/005125.html

Some follow-up is below.

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

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.

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?


+++ lib/Sema/IdentifierResolver.h	(revision 0)

+// This file defines the IdentifierResolver class,which is used for  
lexical

Space after comma, also in the cpp file.

+/// IdentifierResolver - Keeps track of shadowed decls on enclosing  
scopes.
+/// it manages the shadowing chains of identifiers and implements  
efficent decl

Capitalize "It".

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


+++ lib/Sema/IdentifierResolver.cpp	(revision 0)

+/// FETokenInfo of an identifier contains a Decl pointer if lower bit  
== 0

Please end sentences in comments with '.' here and elsewhere.

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


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


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

-Chris