[cfe-commits] r132903 - in /cfe/trunk: lib/Parse/ParseStmt.cpp test/Parser/switch-recovery.cpp

[Douglas Gregor]

On Jun 12, 2011, at 10:50 PM, David Majnemer wrote:

> Author: majnemer
> Date: Mon Jun 13 00:50:12 2011
> New Revision: 132903
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=132903&view=rev
> Log:
> Improve the diagnostics generated for switch statements missing expressions
> 
> - Move the diagnostic to the case statement instead of at the end of the switch
> - Add a fix-it hint as to how to fix the compilation error

I don't consider this a proper use of Fix-Its. If we encounter code like

	switch (z) {
	case 4:
	}

it's almost surely the case that the user got distracted and forgot to write the code for that case, and it's highly unlikely that 

	;

is the right code for this case. Moreover, placing the ';' right after the ':' like this:

	switch (z) {
	case 4:;
	}

is going to hide the issue, especially if we're in a system that automatically applies Fix-Its.

Please revert this patch. I'm sorry that the FIXME there was misleading, but the FIXME is wrong and we shouldn't be suggesting any Fix-Its here because there isn't a clearly correct fix.

	- Doug

> Modified:
>    cfe/trunk/lib/Parse/ParseStmt.cpp
>    cfe/trunk/test/Parser/switch-recovery.cpp
> 
> Modified: cfe/trunk/lib/Parse/ParseStmt.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Parse/ParseStmt.cpp?rev=132903&r1=132902&r2=132903&view=diff
> ==============================================================================
> --- cfe/trunk/lib/Parse/ParseStmt.cpp (original)
> +++ cfe/trunk/lib/Parse/ParseStmt.cpp Mon Jun 13 00:50:12 2011
> @@ -502,6 +502,7 @@
>   StmtTy *DeepestParsedCaseStmt = 0;
> 
>   // While we have case statements, eat and stack them.
> +  SourceLocation ColonLoc;
>   do {
>     SourceLocation CaseLoc = MissingCase ? Expr.get()->getExprLoc() :
>                                            ConsumeToken();  // eat the 'case'.
> @@ -539,7 +540,6 @@
> 
>     ColonProtection.restore();
> 
> -    SourceLocation ColonLoc;
>     if (Tok.is(tok::colon)) {
>       ColonLoc = ConsumeToken();
> 
> @@ -589,8 +589,9 @@
>   } else {
>     // Nicely diagnose the common error "switch (X) { case 4: }", which is
>     // not valid.
> -    // FIXME: add insertion hint.
> -    Diag(Tok, diag::err_label_end_of_compound_statement);
> +    SourceLocation ExpectedLoc = PP.getLocForEndOfToken(ColonLoc);
> +    Diag(ExpectedLoc, diag::err_label_end_of_compound_statement)
> +      << FixItHint::CreateInsertion(ExpectedLoc, ";");
>     SubStmt = true;
>   }
> 
> @@ -634,7 +635,9 @@
> 
>   // Diagnose the common error "switch (X) {... default: }", which is not valid.
>   if (Tok.is(tok::r_brace)) {
> -    Diag(Tok, diag::err_label_end_of_compound_statement);
> +    SourceLocation ExpectedLoc = PP.getLocForEndOfToken(ColonLoc);
> +    Diag(ExpectedLoc, diag::err_label_end_of_compound_statement)
> +      << FixItHint::CreateInsertion(ExpectedLoc, ";");
>     return StmtError();
>   }
> 
> 
> Modified: cfe/trunk/test/Parser/switch-recovery.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/switch-recovery.cpp?rev=132903&r1=132902&r2=132903&view=diff
> ==============================================================================
> --- cfe/trunk/test/Parser/switch-recovery.cpp (original)
> +++ cfe/trunk/test/Parser/switch-recovery.cpp Mon Jun 13 00:50:12 2011
> @@ -156,3 +156,17 @@
>     }
>   }
> }
> +
> +void missing_statement_case(int x) {
> +  switch (x) {
> +    case 1:
> +    case 0: // expected-error {{label at end of compound statement: expected statement}}
> +  }
> +}
> +
> +void missing_statement_default(int x) {
> +  switch (x) {
> +    case 0:
> +    default: // expected-error {{label at end of compound statement: expected statement}}
> +  }
> +}
> 
> 
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits