[PATCH] D63139: [Diagnostics] Implement -Wswitch-unreachable

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/Basic/DiagnosticGroups.td:541
 def SwitchEnum     : DiagGroup<"switch-enum">;
+def SwitchUnreachable     : DiagGroup<"switch-unreachable">;
 def Switch         : DiagGroup<"switch">;
----------------
I don't think you need this group because there's only one diagnostic it controls. You can add the group directly to the warning itself.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, InGroup<SwitchUnreachable>, DefaultIgnore;
 def warn_bool_switch_condition : Warning<
----------------
`InGroup<DiagGroup<"switch-unreachable">>` and drop the `DefaultIgnore`.


================
Comment at: lib/Sema/SemaStmt.cpp:864
 
+  if (CompoundStmt *CS = dyn_cast<CompoundStmt>(BodyStmt)) {
+    for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
----------------
`const auto *`


================
Comment at: lib/Sema/SemaStmt.cpp:865-866
+  if (CompoundStmt *CS = dyn_cast<CompoundStmt>(BodyStmt)) {
+    for (auto It = CS->body_begin(); It != CS->body_end(); ++It) {
+      auto *S = *It;
+      if (isa<LabelStmt>(S) || isa<CaseStmt>(S) || isa<DefaultStmt>(S))
----------------
`for (const Stmt *S : CS->body())`


================
Comment at: lib/Sema/SemaStmt.cpp:871-876
+  } else if (isa<LabelStmt>(BodyStmt) || isa<CaseStmt>(BodyStmt) ||
+             isa<DefaultStmt>(BodyStmt)) {
+    // No warning
+  } else {
+    Diag(BodyStmt->getBeginLoc(), diag::warn_unreachable_stmt_in_switch);
+  }
----------------
You should turn this into a negative predicate rather than having an empty body with an `else` statement.


================
Comment at: test/Sema/switch_unreachable.c:2
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-unreachable %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+
----------------
Remove the `-Wall` since you want to test that this is on by default.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D63139/new/

https://reviews.llvm.org/D63139