[cfe-commits] [PATCH] Improve error recovery for comma/semicolon typo in declaration group

[Richard Smith]

> On Oct 5, 2011, at 5:25 PM, Richard Smith wrote:
>> On Thu, October 6, 2011 00:58, Eli Friedman wrote:
>>> On Wed, Oct 5, 2011 at 4:26 PM, Richard Smith <richard at metafoo.co.uk>
>>> wrote:
>>>> Clang currently gives unhelpful diagnostics for cases such as this:
>>>>
>>>> static const char *const Triples[] = { "powerpc-linux-gnu",
>>>> "powerpc-unknown-linux-gnu"
>>>> },
>>>> CandidateLibDirs.append(LibDirs, LibDirs +
>>>> llvm::array_lengthof(LibDirs));
>>>>
>>>> Viz:
>>>>
>>>> lib/Driver/ToolChains.cpp:1582:7: error: default initialization of an
>>>> object of const type 'const char'     CandidateLibDirs.append(LibDirs,
>>>> LibDirs + llvm::array_lengthof(LibDirs));
>>>> ^
>>>> lib/Driver/ToolChains.cpp:1582:23: error: expected ';' at end of
>>>> declaration CandidateLibDirs.append(LibDirs, LibDirs +
>>>> llvm::array_lengthof(LibDirs));
>>>> ^
>>>> ;
>>>>
>>>>
>>>> The attached patch is a conservative fix for this issue. In cases where
>>>> a declarator group contains a comma followed by a newline followed by
>>>> something which obviously is neither a declarator nor a typo for a
>>>> declarator, we give a fixit suggesting that a ; was intended:
>>>>
>>>> lib/Driver/ToolChains.cpp:1581:8: error: expected ';' at end of
>>>> declaration },
>>>> ^ ;
>>>>
>>>> OK to commit?
>>>
>>> I don't really like Parser::MightBeDeclarator... I can see at least
>>> two cases where it rejects valid code.  Can you use
>>> Parser::TryParseDeclarator
>>> or something?
>>
>> I'd be reluctant to add tentative parsing for all declarator groups with
>> more than one declarator.

On Thu, October 6, 2011 01:44, John McCall wrote:
> Yes, please don't use tentative parsing here. :)  In fact, if you could use
> at most one token of lookahead, that would be ideal.

Indeed, that is how the patch operates.

> Abstractly, I don't really like the approach of opting-out possible
> declarators, but the alternative would make the diagnostic *very*
> special-case.  Please at least leave a comment in the declarator-parsing code
> saying to update your function if a new token is added there, though.

I'm not hugely enamored with the approach either, but I think a black-list of
"obvious" non-declarators would be worse. I've added a comment as you
requested.

On Thu, October 6, 2011 01:59, Eli Friedman wrote:
> On Wed, Oct 5, 2011 at 5:25 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> What are your cases? Even if we change approach, I'd like to add tests for
>> them.
>
> Err, hmm, one of them wasn't actually right.  I'm pretty sure the
> following is valid, though:
>
> int x, y alignas(float);

Thanks! I'd accidentally added kw_alignas to the 'before identifier' list
rather than the 'after identifier' list where it belongs.

The attached patch fixes that, adds support for code_completion tokens, and
adds a few more cases to the whitelist where the existing error recovery was
better than the new error recovery (for instance, in "{ int a, \n b }"). I've
gone over the declaration parsing code again to check I didn't miss any
tokens, and tested this against a very large quantity of C++ code with no
rejects-valids. Further comments welcome!

Thanks,
Richard
-------------- next part --------------
A non-text attachment was scrubbed...
Name: decl-comma-semi-typo-2.diff
Type: text/x-patch
Size: 7267 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20111007/eeaadd8f/attachment.bin>