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

[Richard Smith]

Ping!

On Fri, October 7, 2011 03:50, 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!
>
> Thanks,
> Richard