On May 19, 2011, at 4:14 PM, Richard Trieu wrote:
>
>
> On Sat, May 7, 2011 at 9:59 AM, Douglas Gregor <[email protected]> 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 <[email protected]> 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
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
