Hi again,
On Thu, February 3, 2011 21:05, Douglas Gregor wrote:
> On Jan 25, 2011, at 10:18 AM, Richard Smith wrote:
>> 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 a lot for your comments. After some further experimentation, I
think it would be tricky to get the insert-semicolon fixits reliable
enough. In particular, cases like this are tricky:
for (int n = 0; n < a ++n)
because clang will have parsed the ++ as postfix. The 'insert =' fixit
where the LHS is a DeclRefExpr is unreliable since, as you note, the
initializer (and following semicolon) could simply be missing.
The attached, revised patch has the fixits and the "expected '='"
diagnostic removed. It still resolves the problem where an error within a
for-statement would lead to cascading errors beyond the end of the
for-statement.
Thanks,
Richard
Index: test/Parser/for.cpp
===================================================================
--- test/Parser/for.cpp (revision 0)
+++ test/Parser/for.cpp (revision 0)
@@ -0,0 +1,20 @@
+// RUN: %clang_cc1 -fsyntax-only -verify %s
+
+void f1() {
+ int n;
+
+ for (n = 0; n < 10; n++);
+
+ for (n = 0 n < 10; n++); // expected-error {{expected ';' in 'for'}}
+ for (n = 0; n < 10 n++); // expected-error {{expected ';' in 'for'}}
+
+ for (int n = 0 n < 10; n++); // expected-error {{expected ';' in 'for'}}
+ for (int n = 0; n < 10 n++); // expected-error {{expected ';' in 'for'}}
+
+ for (n = 0 bool b = n < 10; n++); // expected-error {{expected ';' in 'for'}}
+ for (n = 0; bool b = n < 10 n++); // expected-error {{expected ';' in 'for'}}
+
+ for (n = 0 n < 10 n++); // expected-error 2{{expected ';' in 'for'}}
+
+ for (;); // expected-error {{expected ';' in 'for'}}
+}
Index: lib/Parse/ParseStmt.cpp
===================================================================
--- lib/Parse/ParseStmt.cpp (revision 124070)
+++ lib/Parse/ParseStmt.cpp (working copy)
@@ -1039,7 +1039,6 @@
Collection = ParseExpression();
} else {
Diag(Tok, diag::err_expected_semi_for);
- SkipUntil(tok::semi);
}
} else {
Value = ParseExpression();
@@ -1065,8 +1064,14 @@
}
Collection = ParseExpression();
} else {
- if (!Value.isInvalid()) Diag(Tok, diag::err_expected_semi_for);
- SkipUntil(tok::semi);
+ if (!Value.isInvalid()) {
+ Diag(Tok, diag::err_expected_semi_for);
+ } else {
+ // Skip until semicolon or rparen, don't consume it.
+ SkipUntil(tok::r_paren, true, true);
+ if (Tok.is(tok::semi))
+ ConsumeToken();
+ }
}
}
if (!ForEach) {
@@ -1074,6 +1079,8 @@
// Parse the second part of the for specifier.
if (Tok.is(tok::semi)) { // for (...;;
// no second part.
+ } else if (Tok.is(tok::r_paren)) {
+ // missing both semicolons.
} else {
ExprResult Second;
if (getLang().CPlusPlus)
@@ -1088,12 +1095,16 @@
SecondPart = Actions.MakeFullExpr(Second.get());
}
+ if (Tok.isNot(tok::semi)) {
+ if (!SecondPartIsInvalid || SecondVar)
+ Diag(Tok, diag::err_expected_semi_for);
+ else
+ // Skip until semicolon or rparen, don't consume it.
+ SkipUntil(tok::r_paren, true, true);
+ }
+
if (Tok.is(tok::semi)) {
ConsumeToken();
- } else {
- if (!SecondPartIsInvalid || SecondVar)
- Diag(Tok, diag::err_expected_semi_for);
- SkipUntil(tok::semi);
}
// Parse the third part of the for specifier._______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
