[cfe-commits] [PATCH] Improved error recovery in
Richard Smith
richard at metafoo.co.uk
Sun Feb 6 06:05:46 PST 2011
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang-for-2.diff
Type: text/x-patch
Size: 2655 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20110206/e6e9b433/attachment.bin>
More information about the cfe-commits
mailing list