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

[JF Bastien via Phabricator via cfe-commits]

jfb added inline comments.


================
Comment at: include/clang/Basic/DiagnosticSemaKinds.td:8192
+def warn_unreachable_stmt_in_switch : Warning<
+  "statement will be never executed">, InGroup<DiagGroup<"switch-unreachable">>;
 def warn_bool_switch_condition : Warning<
----------------
aaron.ballman wrote:
> I thought we had a warning group for this already, and we do, it's `-Wunreachable-code`. I think the new diagnostic group be a child of the existing one, if we need the group at all.
Agreed.


================
Comment at: lib/Sema/SemaStmt.cpp:727
+                           unsigned UnpromotedWidth, bool UnpromotedSign,
+                           bool &CaseListIsErroneous) {
   // In C++11 onwards, this is checked by the language rules.
----------------
This function is super confusing, and that's not your doing... but adding a `bool&` param kinda piles on. I'd much rather return a `enum class CaseListIsErroneous { No, Yes }`, and make `enum class UnpromotedSign` as well while we're here.


================
Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:29
+  label3:
+    x++;
+  case 4:
----------------
Not that I think you should diagnose here, but I'd like to have a comment in the test saying that it's indeed unreachable, but we don't currently diagnose.


================
Comment at: test/SemaCXX/warn-unreachable-stmt-switch.cpp:49
+  switch (x) {
+    ;
+  case 7:
----------------
Can you also have one like this with typedef and declarations instead of statements:
```
switch (x) {
  using Foo = int;
  case 7:
  // ...
```
and

```
switch (x) {
  int im_confused = 42;
  case 7:
  // ...
```

This one is terrible but reachable:

```
switch (x) {
  ({ oh_no: g(x); });
  case 7:
  goto oh_no;
  // ...
```


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

https://reviews.llvm.org/D63139