[PATCH] D44449: [Parser] Fix assertion-on-invalid for unexpected typename.

[Volodymyr Sapsai via Phabricator via cfe-commits]

vsapsai added inline comments.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3076
         DS.SetRangeEnd(Tok.getAnnotationEndLoc());
         ConsumeAnnotationToken(); // The typename
       }
----------------
rsmith wrote:
> vsapsai wrote:
> > Here we potentially can leave annotation token unconsumed. But I wasn't able to create a test case that would trigger a problem at this point.
> I think you're talking about the `break` a few lines back?
> 
> I think we actually get away with this, but only because `SetTypeSpecType` can only fail if there was already a type specifier, which we checked for on line 3019, so `isInvalid` is never `true`.
You are right, I was talking about `break` on the line 3071.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3148
       DS.SetRangeEnd(Tok.getAnnotationEndLoc());
       ConsumeAnnotationToken(); // The typename
 
----------------
rsmith wrote:
> vsapsai wrote:
> > We didn't consume the annotation token because of `break` on `isInvalid` a few lines above.
> I wonder if our error recovery would be improved by changing line 3134 to just
> 
> ```if (DS.hasTypeSpecifier())```
> 
> (which would likewise render the `break` here unreachable given the current rules enforced by `SetTypeSpecType`).
Change in if condition fixes the assertion too and the diagnostics are
```
repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: expected unqualified-id
2 errors generated.
```

With the currently reviewed change diagnostics are
```
repro.cpp:1:18: error: expected a qualified name after 'typename'
class A typename A;
                 ^
repro.cpp:1:18: error: cannot combine with previous 'class' declaration specifier
2 errors generated.
```

This is for the file
```
$ cat repro.cpp 
class A typename A;
```

This small test isn't representable but I find showing next to each other
> expected a qualified name
> expected unqualified-id
confusing and contradictory at the first glance.

If there are no further comments, I'll commit the change as is (after adding a comment for ConsumeAnyToken).


================
Comment at: clang/lib/Parse/ParseDecl.cpp:3802
     if (DiagID != diag::err_bool_redeclaration)
-      ConsumeToken();
+      ConsumeAnyToken();
 
----------------
rsmith wrote:
> Maybe add a comment here saying we can get here with an annotation token after an error?
Will do.


https://reviews.llvm.org/D44449