[cfe-commits] r41200 - in /cfe/trunk: Parse/ParseObjc.cpp Parse/Parser.cpp include/clang/Basic/DiagnosticKinds.def include/clang/Parse/Parser.h

[Steve Naroff]

On Aug 22, 2007, at 6:21 PM, Chris Lattner wrote:

>
> On Aug 20, 2007, at 2:31 PM, Steve Naroff wrote:
>
>> Author: snaroff
>> Date: Mon Aug 20 16:31:48 2007
>> New Revision: 41200
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=41200&view=rev
>> Log:
>>
>> Start parsing ObjC classes/categories!
>
> Nice!
>
>> +Parser::DeclTy *Parser::ParseObjCAtInterfaceDeclaration(
>> +  SourceLocation atLoc, AttributeList *attrList) {
> ..
>
>> +  if (Tok.getKind() == tok::l_paren) { // we have a category
>
> Comment is a sentence, needs a "." :)
>

Will correct.

>> +    SourceLocation lparenLoc = ConsumeParen();
>> +    SourceLocation categoryLoc, rparenLoc;
>> +    IdentifierInfo *categoryId = 0;
>> +
>> +    // OBJC2: The cateogry name is optional (not an error).
>
> typo: cateogry.
>

Will correct.

> If this is an objc2 feature, should it check getLang().ObjC2?   
> Likewise in other places that handle objc2 features in various ways.
>

I don't have a problem with this. I think it's worth a TODO...not  
important for now.

>> +    if (Tok.getKind() == tok::identifier) {
>> +      categoryId = Tok.getIdentifierInfo();
>> +      categoryLoc = ConsumeToken();
>> +    }
>> +    if (Tok.getKind() != tok::r_paren) {
>> +      Diag(Tok, diag::err_expected_rparen);
>> +      SkipUntil(tok::r_paren, false); // don't stop at ';'
>> +      return 0;
>> +    }
>
> It might be possible to use Parser::MatchRHSPunctuation to handle  
> this.
>
> ...
>
>> +  // Parse a class interface.
>> +  IdentifierInfo *superClassId = 0;
>> +  SourceLocation superClassLoc;
>> +
>> +  if (Tok.getKind() == tok::colon) { // a super class is specified.
>> +    ConsumeToken();
>
> This is a common idiom.  Do you think it would be worthwhile to add a:
>
> bool Parser::ConsumeIfToken(tokenkind), which could be used like:
>
>   if (ConsumeIfToken(tok::colon)) {
>     ...
> ?
>
> However, one bad thing about this is that it loses the location of  
> the token if consumed.

I don't have a strong opinion on this. I am not bothered by the above  
code.

>
>> ===================================================================== 
>> =========
>> --- cfe/trunk/Parse/Parser.cpp (original)
>> +++ cfe/trunk/Parse/Parser.cpp Mon Aug 20 16:31:48 2007
>> @@ -370,6 +370,13 @@
>>      return Actions.ParsedFreeStandingDeclSpec(CurScope, DS);
>>    }
>>
>> +  // OBJC: This grammar hack allows prefix attributes on class  
>> interfaces.
>
> How is this a hack?

Bad terminology. I was a little bothered by calling  
ParseObjCAtInterfaceDeclaration() from this place in the C grammar.

I wish class attributes were after the @interface, but that ship has  
already sailed.

I'll remove the comment.

>
>> +  if (Tok.getKind() == tok::at) {
>> +    SourceLocation AtLoc = ConsumeToken(); // the "@"
>> +    if (Tok.getIdentifierInfo()->getObjCKeywordID() ==  
>> tok::objc_interface)
>
> This is another common idiom.  It seems like it would be nice to  
> add a:
>
> bool Token::isObjCAtKeyword(...)
>
> method, allowing:
>
> if (Tok.isObjCAtKeyword(tok::objc_interface))

I totally agree...this would be a very nice improvement (something I  
thought of as I was writing the code).

In fact, the above code is flawed ("@<non-identifier>" will go boom).

I believe I fixed most of these today, however the code below is even  
more ugly to look at.

+  assert((Tok.getKind() == tok::identifier &&
+          Tok.getIdentifierInfo()->getObjCKeywordID() ==  
tok::objc_interface) &&

So...having a predicate will simplify the code and make sure we don't  
do boom:-)

I can take care of adding this.

snaroff

>
> -Chris
>
>