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
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
Fixed parsing unnamed bit fields
Thank you for example, it represents a case missed from the patch.
Adding colon protector indeed helps for the code:
const int m = 4; struct A { struct n { int m; }; int A::n : m; };
I found another example, that is not MS-specific and is currently
misparsed by clang:
struct A { enum class B { C }; }; const int C = 4; struct D { A::B : C; };
Fixing this case is a bit trickier because declspec is parsed with colon
protection turned off, otherwise we cannot recover typos like:
A:B c;
To recogneze that A::B:C is not a typo for A::B::C we must know that the
parsed scope spec is a part of type specifier.
In the updated patch TryAnnotateCXXScopeToken has additional parameter,
that provide information whether a type is expected, with this change the
above example is parsed correctly.
Fixing this case is a bit trickier because declspec is parsed with colon
protection turned off, otherwise we cannot recover typos like:A:B c;
With your patch, it's parsed with colon protection turned on, right? Or do we also need to add colon protection to tentative parsing of member-declarations?
If you want to detect and recover from the above case, you need more lookahead than you're currently using: you need to check if the colon is followed by *two* identifiers. If it is, you can temporarily turn off colon protection.
lib/Parse/Parser.cpp | ||
---|---|---|
1692–1694 ↗ | (On Diff #10083) | This doesn't seem correct. Even if A::B::C is a type, we still should not do the recovery here, because this may still be an unnamed bitfield. Also, I'm concerned about whether this NeedType thing is correct, since we use TryAnnotateCXXScopeSpec from tentative parsing when we don't know whether we expect a type or not. |
With this patch declspec is parsed with colon protection turned off, this allows make recover of errors like:
struct S2b { C1:C2 f(C1::C2); };
or
struct S6b { C1:C2 m1 : B1::B2; };
Colon protection is turned on when parsing declarator, - when parser expects nested-name-specifier of pointer to a member and when parsing direct-declarator.
If you want to detect and recover from the above case, you need more lookahead than you're currently using: you need to check if the colon is followed by *two* identifiers. If it is, you can temporarily turn off colon protection.
The patch uses a bit different way. If parser parses declspec and sees
A:B
it may be one of two things:
- It is unnamed bit field. In this case A must be a type suitable for bit field and B must be a value.
- It is typo for A::B. In this case A::B must be a type.
In both cases parser must read type first. As there is no case when both A and A::B are types suitable for declaring bit fields, these cases can be distinguished unambiguously, just by trying to build longest nested name specifier, which is still used to refer to a type. If A::B is a type or namespace, A cannot be used as a type of a bit field and recovery A:B -> A::B is valid. Without scoped enums it would be enough to check if A::B exists, that is why we need to pass NeedType when analyzing C++ scope.
lib/Parse/Parser.cpp | ||
---|---|---|
1692–1694 ↗ | (On Diff #10083) | Whether the recovery is attempted is determined by ColonIsSacred field. NeedType only assists the recovering when ColonIsSacred is false. By default NeedType is false and behavior is identical to the current one. In the patch it is set only when parsing declspec, to correctly handle case when A is scoped enum and A::B is valid. |
Parse declspec with colon protection turned on.
It reduces number of cases where colon recovery is possible, but looks more reliable.
Looks great, thanks!
lib/Parse/ParseDecl.cpp | ||
---|---|---|
4531–4534 ↗ | (On Diff #10967) | Please reindent this to make the precedence more obvious (or clang-format it). [Last 3 lines should be one space further to the right.] |