[PATCH] Recovery of switch statement

Richard Smith richard at metafoo.co.uk
Fri Apr 11 13:52:02 PDT 2014



================
Comment at: lib/Parse/ParseStmt.cpp:701-705
@@ -688,5 +700,7 @@
     // not valid.
     SourceLocation AfterColonLoc = PP.getLocForEndOfToken(ColonLoc);
+    if (ColonLoc.isInvalid())
+      AfterColonLoc = PrevTokLocation;
     Diag(AfterColonLoc, diag::err_label_end_of_compound_statement)
       << FixItHint::CreateInsertion(AfterColonLoc, " ;");
     SubStmt = true;
----------------
This doesn't look right in the case where `ColonLoc` is invalid. `AfterColonLoc` will point to the *start* of the previous token in this case, and we'll provide a fix-it inserting a semicolon in a fairly random place.

In the case where `ColonLoc` is invalid, we've already hit at least one error, so it seems better to suppress the follow-on diagnostic here.

I think this will also do the wrong thing if we have a sequence of `case` statements, and the last one is missing a colon or is otherwise malformed; we'll use the last `ColonLoc` for any of the `case` statements we succeeded in handling. Maybe reset `ColonLoc` to `SourceLocation()` each time around the `do` loop to fix this?

================
Comment at: lib/Sema/SemaStmt.cpp:707-708
@@ +706,4 @@
+  if (!Body) {
+    // Put the synthesized null statement on the same line as the end of switch
+    // condition.
+    SourceLocation SynthesizedNullStmtLocation;
----------------
This comment doesn't explain what's happening here. Something like:

  // This happens if the body was ill-formed. Synthesize a null statement at the end of the switch condition.

... would be better. But our normal approach here would be to simply return `StmtError()` in this case. Any reason not to do that?


http://reviews.llvm.org/D3137






More information about the cfe-commits mailing list