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

[David Majnemer]

On Tue, Jun 14, 2011 at 10:41 AM, Douglas Gregor <dgregor at apple.com> wrote:
>
> 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.
I removed the Fix-it in r132994.
>
> 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.
Per discussion on #llvm, I kept the bulk of the patch as it does make
the original diagnostic more clear.
>
>        - 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
>
>