[cfe-commits] r45511 - in /cfe/trunk: AST/Stmt.cpp AST/StmtPrinter.cpp AST/StmtSerialization.cpp Parse/ParseObjc.cpp Parse/Parser.cpp include/clang/AST/Stmt.h include/clang/AST/StmtNodes.def include/clang/Parse/Parser.h

[fjahanian]

All done.

- Fariborz

On Jan 4, 2008, at 2:51 PM, Chris Lattner wrote:

> On Jan 2, 2008, at 2:54 PM, Fariborz Jahanian wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=45511&view=rev
>> Log:
>> New declarations/defs for Objc2's foreach-statement. This is work  
>> in progress.
>
> Nice!
>
> Some really minor thoughts:
>
>> ===================================================================== 
>> =========
>> --- cfe/trunk/Parse/ParseObjc.cpp (original)
>> +++ cfe/trunk/Parse/ParseObjc.cpp Wed Jan  2 16:54:34 2008
>> @@ -491,6 +491,16 @@
>>   return false;
>> }
>>
>> +///  objc-for-collection-in: 'in'
>> +///
>> +bool Parser::isObjCForCollectionInKW() {
>
> I think this method should be const.  Also, would something like  
> "isTokIdentifier_in()" be a better name?  I think it would be best  
> to get "Tok" and "in" in there somehow that makes it more clear  
> what this does.  The fact that this is part of a "for collection"  
> statement isn't really important, it just returns true if the token  
> is an identifier and if it is 'in'.
>
>>
>> +  if (Tok.is(tok::identifier)) {
>> +    const IdentifierInfo *II = Tok.getIdentifierInfo();
>> +    return II == ObjCForCollectionInKW;
>> +  }
>
> Is this more clear than just:
>   if (Tok.is(tok::identifier) &&
>       Tok.getIdentifierInfo() == ObjCForCollectionInKW)
>     return true;
>
> ?
>
>> +/// ObjcForCollectionStmt - This represents Objective-c's  
>> collection statement;
>> +/// represented as 'for (element 'in' collection-expression)' stmt.
>> +///
>> +class ObjcForCollectionStmt : public Stmt {
>> +  enum { ELEM, COLLECTION, BODY, END_EXPR };
>> +  Stmt* SubExprs[END_EXPR]; // SubExprs[ELEM] is an expression or  
>> declstmt.
>> +  SourceLocation ForCollectionLoc;
>
> A really minor nit-pick is that this should probably just be  
> "ForLoc", because it's the location of the 'for' token, not the  
> location of the whole statement.
>
> Otherwise, looking great,
>
> -Chris