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

[Douglas Gregor]

On Oct 6, 2011, at 7:50 PM, Richard Smith wrote:

> 
>> 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!

This LGTM. Clever.

	- Doug