[cfe-commits] [PATCH] PR7606: Diagnostic for ',' instead of '; ' at the end of a statement

[Richard Smith]

Hi,

On Mon, Sep 10, 2012 at 11:15 AM, Ahmed Bougacha
<ahmed.bougacha at gmail.com>wrote:

> On Thu, Sep 6, 2012 at 1:40 AM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > Hi,
> >
> > This patch needs a few more testcases (both positive and negative). I'd
> > particularly like tests which cover the places where C++11 allows a
> braced
> > initializer list, since you're using '{' as an indicator of
> > not-an-assignment-expression.
>
> We added some testcases.


Thanks.


> > Please make isNotExpressionStart be a static function local to
> > ParseExpr.cpp, rather than a member of Parser.
>
> This doesn't make much sense anymore, because we added declaration
> checking, and isDeclarationStatement() is in Parser.
>

OK.


> About declarations, isDeclarationStatement() returns true for "int(f())".
> What do you think should be done in that case? For now, we only check
> declarations when not parsing C++.


In C++, you can use isCXXDeclarationSpecifier(). It will only return
TPResult::True() if you're looking at something which could be the start of
a declaration and can't be the start of an expression.


> > Also, instead of replacing the ',' with a ';' and producing a diagnostic,
> > please just leave the ',' in the token stream and bail out. This will
> allow
> > the appropriate point higher up in the parser to produce the relevant
> > diagnostic (which may not be to replace with a semicolon, depending on
> the
> > context). This affects cases like "if (1, {".
>
> Done!
>

It looks like you're now consuming the ',' before bailing out. I'm not sure
whether that's detectable, but you should leave it in the token stream
anyway so other parts of the compiler can recover better (for instance, we
could offer a ',' -> ';' typo correction in this situation by extending
IsCommonTypo). Given that you're now looking a couple of tokens past the
comma, it'll be best to go back to your former approach of putting it back
into the token stream if you find that it's a typo.


> > Thanks!
> > Richard
>
> Thank you for your help!


Thanks for working on this!
Richard
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120910/c110599a/attachment.html>