[PATCH] Improve error recovery around colon.

[Richard Smith]

On Tue, Jun 10, 2014 at 2:25 AM, Serge Pavlov <sepavloff at gmail.com> wrote:

> >>! In D3653#10, @rsmith wrote:
> >> 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?
> With this patch declspec is parsed with colon protection turned off,


This seems like the wrong design; we should have colon protection turned on
for this. If there are cases where we know it's safe to treat a ':' as '::'
anyway, then that should be special-cased.

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.
>

There's no need for 'B' to be a value here. It could be a type. It could be
a function found by ADL. You can't assume that A:B is not a bit-field by
looking up B.

  - 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.


I'm not sure this argument is correct. However, I think it is correct to
say: if A is non-dependent, and not valid as the type of a bit-field, and
we're parsing the decl-specifiers in a member declaration, then it is safe
to treat A:B as a typo for A::B (doing so will never reject valid code).

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.
>
> ================
> Comment at: lib/Parse/Parser.cpp:1692-1694
> @@ +1691,5 @@
> +///
> +/// Setting NeedType influences error recovery. If it is set, parser
> avoids
> +/// replacements ':' -> '::' if it produces non-type entity. With it we
> can
> +/// parse correctly code like this:
> +/// \code
> ----------------
> Richard Smith wrote:
> > 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.
> 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.
>
> http://reviews.llvm.org/D3653
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140624/be081f33/attachment.html>