<br><br><div class="gmail_quote">On Sat, May 21, 2011 at 9:15 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com">dgregor@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex;">
<div style="word-wrap:break-word"><div><div></div><div class="h5"><br><div><div>On May 19, 2011, at 4:14 PM, Richard Trieu wrote:</div><br><blockquote type="cite"><br><br><div class="gmail_quote">On Sat, May 7, 2011 at 9:59 AM, Douglas Gregor <span dir="ltr"><<a href="mailto:dgregor@apple.com" target="_blank">dgregor@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div><div></div><div><br><div><div>On May 6, 2011, at 6:57 PM, Richard Trieu wrote:</div><br><blockquote type="cite">Moved fix-its from the notes to the error message.<br><br><div class="gmail_quote">
On Fri, May 6, 2011 at 2:20 PM, Richard Trieu <span dir="ltr"><<a href="mailto:rtrieu@google.com" target="_blank">rtrieu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204, 204, 204);border-left-style:solid;padding-left:1ex">When improperly using "::" to nest namespaces, give a fix-it to correct it. Parsing will also recover as if they were properly nested.<div>
<br></div><div>namespace foo::bar {</div><div>}</div><div><br></div><div>
Will result in a fix-it at "::" to change to " { namespace " and an additional right bracket at the end.</div><div><br></div><div>Patch attached and available through Code Review:</div><div><a href="http://codereview.appspot.com/4452052/" target="_blank">http://codereview.appspot.com/4452052/</a></div>
</blockquote></div></blockquote></div><br></div></div><div><div>+ // Unless we have "namepsace foo::bar {", just complain about a lack of '{'.</div><div><br></div><div>Typo "namepsace".</div>
<div><br></div><div>+ if (!Tok.is(tok::coloncolon) || !NextToken().is(tok::identifier)) {</div><div>+ Diag(Tok, Ident ? diag::err_expected_lbrace :</div><div>+ diag::err_expected_ident_lbrace);</div><div>
+ return 0;</div><div>+ }</div></div><div><br></div><div><div>+ // Otherwise, recover parsing as if the user had typed</div><div>+ // "namespace foo { namespace bar {".</div><div>+ TentativeParsingAction TPA(*this);</div>
<div>+ SkipUntil(tok::l_brace, /*StopAtSemi*/false);</div><div>+ SkipUntil(tok::r_brace, /*StopAtSemi*/false, /*DontConsume*/true);</div><div>+ Token rBraceToken = Tok;</div><div>+ rBraceToken.setKind(tok::r_brace);</div>
<div>+ PP.EnterToken(rBraceToken);</div><div>+ TPA.Revert();</div></div><div><br></div><div>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.</div>
<div><br></div><div>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</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>namespace foo::bar {</div><div><span style="white-space:pre-wrap"> </span></div><div><span style="white-space:pre-wrap">a</span>nd 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.</div>
<div><br></div><div>Also, this code is assuming that we're parsing something like</div><div><br></div><div><span style="white-space:pre-wrap"> </span>namespace foo::bar {</div><div><span style="white-space:pre-wrap"> </span>}</div>
<div><br></div><div>but not</div><div><br></div><div><span style="white-space:pre-wrap"> </span>namespace foo::bar::wibble {</div><div><span style="white-space:pre-wrap"> </span>}</div><div><br></div><div>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.</div>
<div><br></div><div><span style="white-space:pre-wrap"> </span>- Doug</div></div></blockquote></div><br><div>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.</div>
</blockquote><br></div></div></div><div>Great! Only two comments, then I think this patch can go in:</div><div><br></div><div><div><div>+ std::string NamespaceFix = "";</div><div>+ for (std::vector<IdentifierInfo*>::iterator I = ExtraIdent.begin(),</div>
<div>+ E = ExtraIdent.end(); I != E; ++I) {</div><div>+ NamespaceFix += " { namespace ";</div><div>+ NamespaceFix += (*I)->getName();</div><div>+ }</div><div>+ std::string RBraces(ExtraIdent.size(), '}');</div>
<div><br></div><div>It's just stylistic, but I'd prefer that that RBraces have spaces between each '}'.</div><div><br></div><div><div>+ // Parse improperly nested namespaces.</div><div>+ ParseScope NamespaceScope(this, Scope::DeclScope);</div>
<div>+ Decl *NamespcDecl =</div><div>+ Actions.ActOnStartNamespaceDef(getCurScope(), InlineLoc,</div><div>+ NamespaceLoc[index], IdentLoc[index],</div><div>+ Ident[index], LBrace, attrs.getList());</div>
</div><div><br></div><div>I think that InlineLoc should only be passed through for the innermost namespace, e.g., given</div><div><br></div><div><span style="white-space:pre-wrap"> </span>inline namespace foo::bar {</div>
<div><span style="white-space:pre-wrap"> </span>}</div><div><br></div><div>we should pass SourceLocation() for the location of "inline" for "foo", but pass InlineLoc for the location of "inline" for "bar".</div>
<div><br></div><div>Thanks for working on this!</div><div><br></div></div></div><div><span style="white-space:pre-wrap"> </span>- Doug</div><div><br></div></div></blockquote></div>Okay, I made the two changes you brought up. All look good?