[cfe-commits] r46524 - in /cfe/trunk: AST/Stmt.cpp AST/StmtSerialization.cpp Parse/ParseObjc.cpp Sema/Sema.h Sema/SemaStmt.cpp include/clang/AST/Stmt.h include/clang/Parse/Action.h

[Chris Lattner]

On Jan 29, 2008, at 11:14 AM, Fariborz Jahanian wrote:

> URL: http://llvm.org/viewvc/llvm-project?rev=46524&view=rev
> Log:
> AST for @synchronized.

Hi Fariborz,

> @@ -1113,7 +1113,27 @@
> ///   @synchronized '(' expression ')'

Please update the grammar production in the comment to include the  
compound stmt that is apparently required by this rule.

>
> ///
> Parser::StmtResult Parser::ParseObjCSynchronizedStmt(SourceLocation  
> atLoc) {
> +  ExprResult Res;

Please sink this down to its initialization...

> +  ConsumeToken(); // consume synchronized
> +  if (Tok.isNot(tok::l_paren)) {
> +    Diag (Tok, diag::err_expected_lparen_after, "@synchronized");
> +    return true;
> +  }
> +  ConsumeParen();  // '('
> +  Res = ParseExpression();

Giving you: ExprResult Res = ParseExpression();

Rationale: it is generally preferred to make the lifetime for a  
variable as short as possible, which makes it marginally easier to  
read the code.  Obviously it isn't a big deal in this case, but it's a  
useful principle.

>
> +  if (Res.isInvalid) {
> +    SkipUntil(tok::semi);
> +    return true;
> +  }
> +  if (Tok.isNot(tok::r_paren)) {
> +    Diag (Tok, diag::err_expected_rparen);
> +    return true;
> +  }
> +  ConsumeParen();  // ')'
> +  StmtResult SynchBody = ParseCompoundStatementBody();

ParseCompoundStatementBody will abort if not given a "{".  Please  
check that one exists before calling it.

Otherwise, looks great!

-Chris