[cfe-commits] Patch: Give a fix-it for improperly nested namespace

[Richard Trieu]

On Sat, May 7, 2011 at 9:59 AM, Douglas Gregor <dgregor at apple.com> wrote:

>
> On May 6, 2011, at 6:57 PM, Richard Trieu wrote:
>
> Moved fix-its from the notes to the error message.
>
> On Fri, May 6, 2011 at 2:20 PM, Richard Trieu <rtrieu at google.com> wrote:
>
>> When improperly using "::" to nest namespaces, give a fix-it to correct
>> it.  Parsing will also recover as if they were properly nested.
>>
>> namespace foo::bar {
>> }
>>
>> Will result in a fix-it at "::" to change to " { namespace " and an
>> additional right bracket at the end.
>>
>> Patch attached and available through Code Review:
>> http://codereview.appspot.com/4452052/
>>
>
> +    // Unless we have "namepsace foo::bar {", just complain about a lack
> of '{'.
>
> Typo "namepsace".
>
> +    if (!Tok.is(tok::coloncolon) || !NextToken().is(tok::identifier)) {
> +      Diag(Tok, Ident ? diag::err_expected_lbrace :
> +           diag::err_expected_ident_lbrace);
> +      return 0;
> +    }
>
> +    // Otherwise, recover parsing as if the user had typed
> +    // "namespace foo { namespace bar {".
> +    TentativeParsingAction TPA(*this);
> +    SkipUntil(tok::l_brace, /*StopAtSemi*/false);
> +    SkipUntil(tok::r_brace, /*StopAtSemi*/false, /*DontConsume*/true);
> +    Token rBraceToken = Tok;
> +    rBraceToken.setKind(tok::r_brace);
> +    PP.EnterToken(rBraceToken);
> +    TPA.Revert();
>
> IMO, this isn't robust enough for a Fix-It, because it isn't actually
> checking for the presence of the '{' or the '}'. For example, if the
> "namespace foo::bar" was at the end of the translation unit, it looks like
> we could end up trying to change the end-of-file token to a right-brace.
>
> I think it's fine to use tentative parsing here, but it should only be used
> to identify where the '{' and '}' tokens are so we know that this could be a
> well-formed namespace definition. Once we've figured that out, I'd prefer
> that the code not try to change the token stream. Rather, I think it would
> be better just to parse the
>
> namespace foo::bar {
> and call the appropriate semantic actions to start the foo namespace, then
> the bar namespace, and keep track of the results of each of those calls.
> Then, when we see the closing '}', call the appropriate semantic actions to
> complete those namespace definitions.
>
> Also, this code is assuming that we're parsing something like
>
> namespace foo::bar {
> }
>
> but not
>
> namespace foo::bar::wibble {
> }
>
> Why not generalize it to an arbitrary-length identifier-and-:: definitions?
> If this were a language feature, we'd want it to support an arbitrary depth.
>
> - Doug
>

Changed how the error recovery works.  Tentative parsing is still used to
locate the closing '}', but the token stream is no longer altered.  Instead
of an error for every nested namespace, only produce one error overall with
the fix-its.  Error message will also trigger, but without the fix-its, if
the namespace is not properly formed.  Arbitrary-length nested-namespaces
are now handled.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110519/6bb6930f/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improperly_nested_namespaces3.patch
Type: text/x-patch
Size: 8090 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110519/6bb6930f/attachment.bin>