Hi Richard,
On Jan 25, 2011, at 10:18 AM, Richard Smith wrote:
> Hi,
>
> The attached patch improves diagnostics error recovery in for-statements.
> Examples:
>
> [snip examples]
> The patch can be viewed online here: http://codereview.appspot.com/3992046/
>
> Please let me know what I need to fix in order to get this committed.
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 124070)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -14,6 +14,8 @@
#include "clang/Parse/Parser.h"
#include "RAIIObjectsForParser.h"
+#include "clang/AST/Expr.h"
+#include "clang/AST/Decl.h"
#include "clang/Sema/DeclSpec.h"
#include "clang/Sema/PrettyDeclStackTrace.h"
#include "clang/Sema/Scope.h"
@@ -1038,8 +1040,22 @@
}
Collection = ParseExpression();
} else {
- Diag(Tok, diag::err_expected_semi_for);
- SkipUntil(tok::semi);
+ // suggest 'expected "="' if the declaration's last declarator lacks
+ // an initializer. eg. for (int n 0; n < 10; ++n)
+ VarDecl *VD = DG.get().isNull() ? 0 : dyn_cast<VarDecl>(*(DG.get().end()
- 1));
+ if (VD && !VD->hasInit()) {
+ Diag(Tok, diag::err_expected_equals_for)
+ << FixItHint::CreateInsertion(Tok.getLocation(), "=");
+ ExprResult Init = ParseExpression();
+ VD->setInit(Init.take());
+ }
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).
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.
+ if (Tok.is(tok::semi)) {
+ ConsumeToken();
+ } else {
+ // eg. for (int n = 0l n < 10; ++n)
+ Diag(Tok, diag::err_expected_semi_for)
+ << FixItHint::CreateInsertion(Tok.getLocation(), "; ");
+ }
}
Again, we need a higher degree of confidence that inserting the semicolon is
actually the right thing to do.
@@ -1065,8 +1081,30 @@
}
Collection = ParseExpression();
} else {
- if (!Value.isInvalid()) Diag(Tok, diag::err_expected_semi_for);
- SkipUntil(tok::semi);
+ if (!Value.isInvalid()) {
+ // 'expected "="' if the expression is a DeclRefExpr: for (n 0; n <
10; ++n)
+ if (isa<DeclRefExpr>(Value.get())) {
+ SourceLocation TokLocation = Tok.getLocation();
+ Diag(Tok, diag::err_expected_equals_for)
+ << FixItHint::CreateInsertion(TokLocation, "=");
+ ExprResult Init = ParseExpression();
+ if (!Init.isInvalid()) {
+ Value = Actions.ActOnBinOp(getCurScope(), TokLocation, tok::equal,
+ Value.take(), Init.take());
+ FirstPart =
Actions.ActOnExprStmt(Actions.MakeFullExpr(Value.get()));
+ }
+ }
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 '='.
Thanks for working on this!
- Doug_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
