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

[Richard Smith]

On Wed, October 19, 2011 06:24, Douglas Gregor wrote:
> 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.

Thanks, in r142544.