<html><head></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">Hi Richard,<div><br><div><div>On Jan 25, 2011, at 10:18 AM, Richard Smith wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div>Hi,<br><br>The attached patch improves diagnostics error recovery in for-statements.<br>Examples:<br><br>[snip examples]<br></div></blockquote><br><blockquote type="cite"><div>The patch can be viewed online here: <a href="http://codereview.appspot.com/3992046/">http://codereview.appspot.com/3992046/</a><br><br>Please let me know what I need to fix in order to get this committed.<br></div></blockquote></div><div><br></div><div><div>Index: lib/Parse/ParseStmt.cpp</div><div>===================================================================</div><div>--- lib/Parse/ParseStmt.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(revision 124070)</div><div>+++ lib/Parse/ParseStmt.cpp<span class="Apple-tab-span" style="white-space:pre"> </span>(working copy)</div><div>@@ -14,6 +14,8 @@</div><div> </div><div> #include "clang/Parse/Parser.h"</div><div> #include "RAIIObjectsForParser.h"</div><div>+#include "clang/AST/Expr.h"</div><div>+#include "clang/AST/Decl.h"</div><div> #include "clang/Sema/DeclSpec.h"</div><div> #include "clang/Sema/PrettyDeclStackTrace.h"</div><div> #include "clang/Sema/Scope.h"</div><div>@@ -1038,8 +1040,22 @@</div><div> }</div><div> Collection = ParseExpression();</div><div> } else {</div><div>- Diag(Tok, diag::err_expected_semi_for);</div><div>- SkipUntil(tok::semi);</div><div>+ // suggest 'expected "="' if the declaration's last declarator lacks</div><div>+ // an initializer. eg. for (int n 0; n < 10; ++n)</div><div>+ VarDecl *VD = DG.get().isNull() ? 0 : dyn_cast<VarDecl>(*(DG.get().end() - 1));</div><div>+ if (VD && !VD->hasInit()) {</div><div>+ Diag(Tok, diag::err_expected_equals_for)</div><div>+ << FixItHint::CreateInsertion(Tok.getLocation(), "=");</div><div>+ ExprResult Init = ParseExpression();</div><div>+ VD->setInit(Init.take());</div><div>+ }</div><div><br></div><div>There are a couple problems with this… first of all, we shouldn't suggest a fix-it unless we're fairly certain that it's correct. The '=' here is a bit too much of a guess, since we have no idea that we're going to get two expressions in a row. The user might have forgotten the initializer completely, rather than forgetting the semicolon. Perhaps a bit more lookahead would make it possible to distinguish these cases, so that the Fix-It can be right almost all of the time (which is our criteria for this feature).</div><div><br></div><div>Second, just setting the initializer with VD->setInit() is going to cause huge problems down the line, because the initializer needs to be checked and converted by Sema. At the very least, we'd need a call to Sema::AddInitializerToDecl. However, this happens too late in semantic analysis (after Sema::ActOnUnitializedDecl is called) for this to actually work. For example, we will already have complained if the variable has reference type or has no valid default constructor.</div><div><br></div><div>+ if (<a href="http://Tok.is">Tok.is</a>(tok::semi)) {</div><div>+ ConsumeToken();</div><div>+ } else {</div><div>+ // eg. for (int n = 0l n < 10; ++n)</div><div>+ Diag(Tok, diag::err_expected_semi_for)</div><div>+ << FixItHint::CreateInsertion(Tok.getLocation(), "; ");</div><div>+ }</div><div> }</div></div><div><br></div>Again, we need a higher degree of confidence that inserting the semicolon is actually the right thing to do.</div><div><br></div><div><div>@@ -1065,8 +1081,30 @@</div><div> }</div><div> Collection = ParseExpression();</div><div> } else {</div><div>- if (!Value.isInvalid()) Diag(Tok, diag::err_expected_semi_for);</div><div>- SkipUntil(tok::semi);</div><div>+ if (!Value.isInvalid()) {</div><div>+ // 'expected "="' if the expression is a DeclRefExpr: for (n 0; n < 10; ++n)</div><div>+ if (isa<DeclRefExpr>(Value.get())) {</div><div>+ SourceLocation TokLocation = Tok.getLocation();</div><div>+ Diag(Tok, diag::err_expected_equals_for)</div><div>+ << FixItHint::CreateInsertion(TokLocation, "=");</div><div>+ ExprResult Init = ParseExpression();</div><div>+ if (!Init.isInvalid()) {</div><div>+ Value = Actions.ActOnBinOp(getCurScope(), TokLocation, tok::equal,</div><div>+ Value.take(), Init.take());</div><div>+ FirstPart = Actions.ActOnExprStmt(Actions.MakeFullExpr(Value.get()));</div><div>+ }</div><div>+ }</div><div><br></div><div>Same general comment; we need more lookahead before we can determine that the user actually meant to write the assignment in here. The use of ActOnBinOp/ActOnExprStmt is great here, though, because it's doing semantic analysis to recovery as if the user had written the '='.</div><div><br></div><div>Thanks for working on this!</div><div><br></div><div><span class="Apple-tab-span" style="white-space:pre"> </span>- Doug</div></div></body></html>