[cfe-commits] r42452 - in /cfe/trunk: AST/Expr.cpp Lex/IdentifierTable.cpp Parse/ParseObjc.cpp Sema/Sema.h Sema/SemaDecl.cpp Sema/SemaExpr.cpp clang.xcodeproj/project.pbxproj include/clang/AST/ASTContext.h include/clang/AST/Decl.h include/clang/AST/Expr.h include/clang/Lex/IdentifierTable.h include/clang/Lex/Preprocessor.h include/clang/Parse/Action.h include/clang/Parse/Parser.h

[Chris Lattner]

On Sep 28, 2007, at 3:22 PM, Steve Naroff wrote:
> Author: snaroff
> Date: Fri Sep 28 17:22:11 2007
> New Revision: 42452
>
> URL: http://llvm.org/viewvc/llvm-project?rev=42452&view=rev
> Log:
>
> Yesterday I discovered that 78% of all selectors in "Cocoa.h" take  
> 0/1 argument.
>
> This motivated implementing a devious clattner inspired solution:-)
>
> This approach uses a small value "Selector" class to point to an  
> IdentifierInfo for the 0/1 case. For multi-keyword selectors, we  
> instantiate a MultiKeywordSelector object (previously known as  
> SelectorInfo). Now, the incremental cost for selectors is only  
> 24,800 for Cocoa.h! This saves 156,592 bytes, or 86%!! The size  
> reduction is also the result of getting rid of the AST slot, which  
> was not strictly necessary (we will associate a selector with it's  
> method using another table...most likely in Sema).

Very nice Steve!  Some comments:

> ====================================================================== 
> ========
> --- cfe/trunk/include/clang/Lex/IdentifierTable.h (original)
> +++ cfe/trunk/include/clang/Lex/IdentifierTable.h Fri Sep 28  
> 17:22:11 2007
> @@ -170,28 +170,24 @@
>    void AddKeywords(const LangOptions &LangOpts);
>  };
>
> +/// MultiKeywordSelector - One of these variable length records is  
> kept for each
> +/// parsed selector (similar in spirit to IdentifierInfo). We use  
> a folding set
> +/// to unique aggregate names (keyword selectors in ObjC parlance).
> +class MultiKeywordSelector : public llvm::FoldingSetNode {

I don't think there is any reason to export the definition of  
MultiKeywordSelector in the header file.  Can you make it just  
forward-declared here and suck the definition/implementation into  
IdentifierTable.cpp?  Clients should never interact with  
MultiKeywordSelector, they should only interact with Selector.

Ahh, I see you added this later:
/// The only reason it appears in this header is FoldingSet needs to  
see it:-(

Presumably this is because Preprocessor contains:
   /// Selectors - This table contains all the selectors in the  
program. Unlike
   /// IdentifierTable above, this table *isn't* populated by the  
preprocessor.
   /// It is declared/instantiated here because it's role/lifetime is
   /// conceptually similar the IdentifierTable. In addition, the  
current control
   /// flow (in clang::ParseAST()), make it convenient to put here.
   /// FIXME: Make sure the lifetime of Identifiers/Selectors *isn't*  
tied to
   /// the lifetime fo the preprocessor.
   llvm::FoldingSet<MultiKeywordSelector> Selectors;

This declaration is unfortunate for two reasons:
1. it requires MultiKeywordSelector to be exported out of  
IdentifierTable.cpp, and
2. it requires preprocessor and IdentifierTable.h to #include  
FoldingSet.h

Instead of doing this, I'd suggest adding the following to  
IdentifierTable.h:

class SelectorUniquingTable {
   void *Impl;  // Actually a FoldingSet<MultiKeywordSelector>*
   SelectorUniquingTable(const SelectorUniquingTable&); // DISABLED:  
DO NOT IMPLEMENT
   void operator=(const SelectorUniquingTable&); // DISABLED: DO NOT  
IMPLEMENT
public:
   SelectorUniquingTable();  // new's the foldingset
   ~SelectorUniquingTable();  // delete's the foldingset

   Selector getSelector(unsigned nKeys, IdentifierInfo **IIV);
   Selector getUnarySelector(IdentifierInfo *ID);
   Selector getNullarySelector(IdentifierInfo *ID);
};

With this definition, Parser can contain an instance of  
SelectorUniquingTable instead of a folding set, getting rid of the  
#include of FoldingSet.h and allowing MultiKeywordSelector to become  
private to the .cpp file.

Another benefit is that stuff like this (from Selector and  
MultiKeywordSelector):
> +  friend class Parser; // only the Parser can create these.

can now befriend the SelectorUniquingTable class instead, fixing a  
layering problem (because ideally IdentifierTable will sink into  
libbasic).


> +  IdentifierInfo *getIdentifierInfoForSlot(unsigned i) {
> +    assert((i < NumArgs) && "getIdentifierInfoForSlot(): illegal  
> index");
> +    return keyword_begin()[i];
> +  }

This method should be const.

+  friend class llvm::FoldingSet<MultiKeywordSelector>;

This can go away when it is in the .cpp file, because all the access  
can be public.

> +class Selector {

The selector class needs a comment above it.

> +  // Derive the full selector name, placing the result into  
> methodBuffer.
> +  // As a convenience, a pointer to the first character is returned.
> +  // Example usage: llvm::SmallString<128> mbuf; Selector->getName 
> (mbuf);
> +  char *getName(llvm::SmallVectorImpl<char> &methodBuffer);

Why is this implemented in ParseObjc.cpp?  It seems like it should be  
in IdentifierTable.cpp.

>
> Modified: cfe/trunk/Sema/SemaDecl.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/Sema/ 
> SemaDecl.cpp?rev=42452&r1=42451&r2=42452&view=diff
>
> ====================================================================== 
> ========
> --- cfe/trunk/Sema/SemaDecl.cpp (original)
> +++ cfe/trunk/Sema/SemaDecl.cpp Fri Sep 28 17:22:11 2007
>
> @@ -1206,35 +1206,35 @@
>  static void ImplMethodsVsClassMethods(Sema* objSema,
>  				      ObjcImplementationDecl* IMPDecl,
>                                        ObjcInterfaceDecl* IDecl) {
> -  llvm::DenseMap<const SelectorInfo*, char> InsMap;
> +  llvm::DenseMap<void *, char> InsMap;

Since this code was written, a new llvm::DenseSet was added.  You  
should be able to switch this to use DenseSet, which makes it more  
obvious what you're doing here.

Also, instead of using .getAsOpaquePtr(), I think it would make sense  
to teach the hash table how to handle Selectors.  It seems useful to  
other people and can be added easily to IdentifierInfo.h.

> ====================================================================== 
> ========
> --- cfe/trunk/include/clang/AST/Expr.h (original)
> +++ cfe/trunk/include/clang/AST/Expr.h Fri Sep 28 17:22:11 2007
> @@ -1100,12 +1101,17 @@
>    Expr *getReceiver() { return SubExprs[RECEIVER]; }
>
>    /// getNumArgs - Return the number of actual arguments to this  
> call.
> +  unsigned getNumArgs() const { return SelName.getNumArgs(); }
> +
> +/// getArg - Return the specified argument.
> +  Expr *getArg(unsigned Arg) {
> +    assert(Arg < SelName.getNumArgs() && "Arg access out of range!");
> +    return SubExprs[Arg+ARGS_START];
> +  }

Funky comment indentation.

Overall, huge leap forward!

-Chris