[PATCH] Improve error recovery around colon.

[Richard Smith]

On Fri, May 23, 2014 at 10:47 AM, Serge Pavlov <sepavloff at gmail.com> wrote:

>
>
>
> 2014-05-23 6:43 GMT+07:00 Richard Smith <richard at metafoo.co.uk>:
>
> --- lib/Parse/ParseDecl.cpp
>> +++ lib/Parse/ParseDecl.cpp
>> @@ -2706,7 +2706,7 @@
>>      case tok::identifier: {
>>        // In C++, check to see if this is a scope specifier like
>> foo::bar::, if
>>        // so handle it as such.  This is important for ctor parsing.
>> -      if (getLangOpts().CPlusPlus) {
>> +      if (getLangOpts().CPlusPlus && !DS.hasTypeSpecifier()) {
>>
>> Please instead swap over this 'if' and the next one.
>>
>>
> Removed unneded check.
>
>
>>
>> @@ -4508,8 +4508,10 @@
>>    // Member pointers get special handling, since there's no place for the
>>    // scope spec in the generic path below.
>>    if (getLangOpts().CPlusPlus &&
>> -      (Tok.is(tok::coloncolon) || Tok.is(tok::identifier) ||
>> -       Tok.is(tok::annot_cxxscope))) {
>> +      (Tok.is(tok::coloncolon) ||
>> +      (Tok.is(tok::identifier) && (NextToken().is(tok::coloncolon) ||
>> +                                   NextToken().is(tok::less))) ||
>> +      Tok.is(tok::annot_cxxscope))) {
>>
>> I don't think this is quite sufficient. If we have:
>>
>>   struct A { enum E {}; };
>>   struct B {
>>     static const int N = 3;
>>     A::E : N;
>>   };
>>
>> ... I think we might treat the : as a :: typo. Instead, drop a
>> ColonProtectionRAII object into this 'if', with a comment saying you're
>> doing it for bitfields, if you're in a member context.
>>
>
> Not sure if I undestand correctly. This piece of code is correcly parsed
> as unnamed bit field of enum type. Testcase lacked for tests on unnamed
> bitfields, they are added now.
>

You may need to try a bit harder to get this to misparse, but I think it
will. The point is that we're not protecting colons when we parse the scope
specifier, and the check:

-      (Tok.is(tok::coloncolon) || Tok.is(tok::identifier) ||
-       Tok.is(tok::annot_cxxscope))) {
+      (Tok.is(tok::coloncolon) ||
+      (Tok.is(tok::identifier) && (NextToken().is(tok::coloncolon) ||
+                                   NextToken().is(tok::less))) ||
+      Tok.is(tok::annot_cxxscope))) {

only protects a colon at the start, not a colon *after* the scope. Here's
another attempt to get this to fail:

  const int m = 4; struct A { struct n { int m; }; int A::n : m; };

This code is valid with -fms-extensions, but I think you might correct the
':' to a '::' here.

What I'm suggesting is that you revert the above change and add in
colon-protection around the parsing of the scope specifier. Other than
that, this patch looks great!

On Fri, May 9, 2014 at 4:17 AM, Serge Pavlov <sepavloff at gmail.com> wrote:
>>
>>> Hi Richard,
>>>
>>> Could you please review this fix?
>>> Thank you.
>>>
>>> --Serge
>>>
>>> Recognize additional cases, when '::' is mistyped as ':' and provide
>>> descriptive diagnostics for them.
>>> This is a fix to RP18587 - colons have too much protection in
>>> member-declarations
>>>
>>> http://reviews.llvm.org/D3653
>>>
>>> Files:
>>>   lib/Parse/ParseDecl.cpp
>>>   lib/Parse/ParseDeclCXX.cpp
>>>   test/SemaCXX/nested-name-spec.cpp
>>>
>>>
>>
>
>
> --
> Thanks,
> --Serge
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140530/d84e04b9/attachment.html>