[cfe-commits] r42643 - in /cfe/trunk: Lex/IdentifierTable.cpp Parse/ParseObjc.cpp clang.xcodeproj/project.pbxproj include/clang/AST/ASTContext.h include/clang/Lex/IdentifierTable.h include/clang/Lex/Preprocessor.h include/clang/Parse/Parser.h

[Chris Lattner]

> - Add SelectorTable, which enables us to remove  
> MultiKeywordSelector from the public header.
> - Remove FoldingSet from IdentifierInfo.h and Preprocessor.h.
> - Remove Parser::ObjcGetUnarySelector and  
> Parser::ObjcGetKeywordSelector, they are subsumed by SelectorTable.
> - Add MultiKeywordSelector to IdentifierInfo.cpp.
> - Move a bunch of selector related methods from ParseObjC.cpp to  
> IdentifierInfo.cpp.
> - Added some comments.

Very nice, a couple of further refinements:
> ====================================================================== 
> ========
> --- cfe/trunk/Lex/IdentifierTable.cpp (original)
> +++ cfe/trunk/Lex/IdentifierTable.cpp Fri Oct  5 13:42:47 2007
> +class MultiKeywordSelector : public llvm::FoldingSetNode {
>
...

> +  static void Profile(llvm::FoldingSetNodeID &ID,
> +                      keyword_iterator ArgTys, unsigned NumArgs) {
> +    ID.AddInteger(NumArgs);
> +    if (NumArgs) { // handle keyword selector.
> +      for (unsigned i = 0; i != NumArgs; ++i)
> +        ID.AddPointer(ArgTys[i]);
> +    } else // handle unary selector.
> +      ID.AddPointer(ArgTys[0]);
> +  }

This shouldn't need to handle zero/one arg    selectors.

> +  void Profile(llvm::FoldingSetNodeID &ID) {
> +    Profile(ID, keyword_begin(), NumArgs);
> +  }
> +};
> +
> +IdentifierInfo *Selector::getIdentifierInfoForSlot(unsigned  
> argIndex) {
> +  IdentifierInfo *II = getAsIdentifierInfo();
> +  if (II) {


You can sink the declaration of II into the if.

> +    assert(((argIndex == 0) || (argIndex == 1)) && "illegal  
> keyword index");

Is this assert right?  it should only allow argindex = 0, no?

> +char *MultiKeywordSelector::getName(llvm::SmallVectorImpl<char>  
> &methodName) {
> +  methodName[0] = '\0';
> +  keyword_iterator KeyIter = keyword_begin();
> +  for (unsigned int i = 0; i < NumArgs; i++) {
> +    if (KeyIter[i]) {

Plz convert to iterator increment.

> +char *Selector::getName(llvm::SmallVectorImpl<char> &methodName) {
> +  methodName[0] = '\0';

This store is invalid.

> +  IdentifierInfo *II = getAsIdentifierInfo();
> +  if (II) {

Similar comments to above:

> Modified: cfe/trunk/Parse/ParseObjc.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/Parse/ 
> ParseObjc.cpp?rev=42643&r1=42642&r2=42643&view=diff
>
> ====================================================================== 
> ========
> --- cfe/trunk/Parse/ParseObjc.cpp (original)
> +++ cfe/trunk/Parse/ParseObjc.cpp Fri Oct  5 13:42:47 2007
> @@ -680,8 +597,11 @@
>      // If attributes exist after the method, parse them.
>      if (getLang().ObjC2 && Tok.getKind() == tok::kw___attribute)
>        methodAttrs = ParseAttributes();
> -
> -    Selector Sel = ObjcGetKeywordSelector(KeyIdents);
> +
> +    unsigned nKeys = KeyIdents.size();
> +    Selector Sel = (nKeys == 1) ?
> +      PP.getSelectorTable().getUnarySelector(KeyIdents[0]) :
> +      PP.getSelectorTable().getKeywordSelector(nKeys, &KeyIdents[0]);

Would it be reasonable to add a getSelector(Num, II**) method that  
returned a unary selector if Num is 0?

> @@ -1290,21 +1210,24 @@
>    }
>    SourceLocation RBracloc = ConsumeBracket(); // consume ']'
>
> +  unsigned nKeys = KeyIdents.size();
> +  if (nKeys) {

The variable decl can be sunk into the if condition.

> +    Selector Sel = (nKeys == 1) ?
> +      PP.getSelectorTable().getUnarySelector(KeyIdents[0]) :
> +      PP.getSelectorTable().getKeywordSelector(nKeys, &KeyIdents[0]);

This could use a unified getSelector() method.

-Chris