[PATCH] D44449: [Parser] Fix assertion-on-invalid for unexpected typename.
Volodymyr Sapsai via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Fri Apr 6 15:36:56 PDT 2018
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
More information about the cfe-commits
mailing list