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

[Richard Trieu]

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

>
> On May 19, 2011, at 4:14 PM, Richard Trieu wrote:
>
>
>
> 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.
>
>
> Great! Only two comments, then I think this patch can go in:
>
> +      std::string NamespaceFix = "";
> +      for (std::vector<IdentifierInfo*>::iterator I = ExtraIdent.begin(),
> +           E = ExtraIdent.end(); I != E; ++I) {
> +        NamespaceFix += " { namespace ";
> +        NamespaceFix += (*I)->getName();
> +      }
> +      std::string RBraces(ExtraIdent.size(), '}');
>
> It's just stylistic, but I'd prefer that that RBraces have spaces between
> each '}'.
>
> +  // Parse improperly nested namespaces.
> +  ParseScope NamespaceScope(this, Scope::DeclScope);
> +  Decl *NamespcDecl =
> +    Actions.ActOnStartNamespaceDef(getCurScope(), InlineLoc,
> +                                   NamespaceLoc[index], IdentLoc[index],
> +                                   Ident[index], LBrace, attrs.getList());
>
> I think that InlineLoc should only be passed through for the innermost
> namespace, e.g., given
>
> inline namespace foo::bar {
> }
>
> we should pass SourceLocation() for the location of "inline" for "foo", but
> pass InlineLoc for the location of "inline" for "bar".
>
> Thanks for working on this!
>
> - Doug
>
> Okay, I made the two changes you brought up.  All look good?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110523/7f1ddf29/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: improperly_nested_namespaces4.patch
Type: text/x-patch
Size: 8162 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110523/7f1ddf29/attachment.bin>