[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D63192#1578867 , @xbolva00 wrote:

> I see. I should really check clang-tidy codebase :)


I have a hunch that this will be equally as trivially implementable in 
clang-tidy -- if you go that route, feel free to add me as a reviewer!


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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 planned changes to this revision.
xbolva00 added a comment.

I see. I should really check clang-tidy codebase :)


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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

I'm wary of adding this as an on-by-default warning, despite its presence in 
MSVC (and despite submitting a similar patch years ago to implement the same 
functionality). Machine-generated code runs into this one frequently (we've 
been bitten by it numerous times with our tablegen code, especially in the 
backends), but it does not seem likely to occur in hand-written code very often.

While it's pretty easy to implement this in the frontend, I sort of feel like 
this is a better candidate for clang-tidy's `readability` module.


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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 208984.

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

https://reviews.llvm.org/D63192

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/Sema/switch-availability.c
  test/SemaCXX/switch_default.cpp
  test/SemaCXX/uninitialized.cpp

Index: test/SemaCXX/uninitialized.cpp
===
--- test/SemaCXX/uninitialized.cpp
+++ test/SemaCXX/uninitialized.cpp
@@ -1444,8 +1444,8 @@
   if (int n = 0; (n == k || k > 5)) {}
 
   if (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n = 0; (n == k || k > 5)) {} // expected-warning {{boolean}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}} expected-warning {{boolean}}
 }
Index: test/SemaCXX/switch_default.cpp
===
--- test/SemaCXX/switch_default.cpp
+++ test/SemaCXX/switch_default.cpp
@@ -0,0 +1,38 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void test(int x) {
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+break;
+  }
+
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+x++;
+  }
+
+  switch (x) {
+  default:; // expected-warning {{switch contains only default label}}
+  }
+
+  switch (x)
+  default: // expected-warning {{switch contains only default label}}
+break;
+
+  switch (x) {
+  default:
+return;
+  case 1:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+return;
+  }
+}
Index: test/Sema/switch-availability.c
===
--- test/Sema/switch-availability.c
+++ test/Sema/switch-availability.c
@@ -5,7 +5,7 @@
 };
 
 void testSwitchOne(enum SwitchOne so) {
-  switch (so) {} // no warning
+  switch (so) {} // expected-warning {{switch has no default and case labels}}
 }
 
 enum SwitchTwo {
@@ -15,6 +15,7 @@
 };
 
 void testSwitchTwo(enum SwitchTwo st) {
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (st) {} // expected-warning{{enumeration values 'Vim' and 'Emacs' not handled in switch}}
 }
 
@@ -23,5 +24,6 @@
 };
 
 void testSwitchThree(enum SwitchThree st) {
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (st) {} // expected-warning{{enumeration value 'New' not handled in switch}}
 }
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -865,8 +865,13 @@
 
   bool CaseListIsErroneous = false;
 
-  for (SwitchCase *SC = SS->getSwitchCaseList(); SC && !HasDependentValue;
-   SC = SC->getNextSwitchCase()) {
+  SwitchCase *SC = SS->getSwitchCaseList();
+  if (!SC)
+Diag(SS->getBeginLoc(), diag::warn_no_cases_in_switch);
+  else if (isa(SC) && !SC->getNextSwitchCase())
+Diag(SC->getBeginLoc(), diag::warn_default_only_in_switch);
+
+  for (; SC && !HasDependentValue; SC = SC->getNextSwitchCase()) {
 
 if (DefaultStmt *DS = dyn_cast(SC)) {
   if (TheDefaultStmt) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8201,6 +8201,10 @@
 def warn_break_binds_to_switch : Warning<
   "'break' is bound to loop, GCC binds it to switch">,
   InGroup;
+def warn_no_cases_in_switch : Warning<
+  "switch has no default and case labels">, InGroup, DefaultIgnore;
+def warn_default_only_in_switch : Warning<
+  "switch contains only default label">, InGroup, DefaultIgnore;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -458,7 +458,6 @@
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
-def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
@@ -537,6 +536,7 @@
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : 

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 added a comment.

In D63192#1539605 , @xbolva00 wrote:

> I will work on this after we land https://reviews.llvm.org/D63139.


^ not yet landed, but please @aaron.ballman  take a look, this patch is ready 
for review.


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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 updated this revision to Diff 208974.
xbolva00 added a comment.

Rebased, improved.


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

https://reviews.llvm.org/D63192

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/SemaCXX/switch_default.cpp
  test/SemaCXX/uninitialized.cpp

Index: test/SemaCXX/uninitialized.cpp
===
--- test/SemaCXX/uninitialized.cpp
+++ test/SemaCXX/uninitialized.cpp
@@ -1444,8 +1444,8 @@
   if (int n = 0; (n == k || k > 5)) {}
 
   if (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n = 0; (n == k || k > 5)) {} // expected-warning {{boolean}}
-
+  // expected-warning@+1 {{switch has no default and case labels}}
   switch (int n; (n == k || k > 5)) {} // expected-warning {{uninitialized}} expected-note {{initialize}} expected-warning {{boolean}}
 }
Index: test/SemaCXX/switch_default.cpp
===
--- test/SemaCXX/switch_default.cpp
+++ test/SemaCXX/switch_default.cpp
@@ -0,0 +1,41 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void test(int x) {
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+break;
+  }
+
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+x++;
+  }
+
+  switch (x) {
+  default:; // expected-warning {{switch contains only default label}}
+  }
+
+  switch (x)
+  default: // expected-warning {{switch contains only default label}}
+break;
+
+  switch (x) {
+  default:
+return;
+  case 1:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+return;
+  }
+
+  switch (x) { // expected-warning {{switch has no default and case labels}}
+  }
+}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -865,8 +865,13 @@
 
   bool CaseListIsErroneous = false;
 
-  for (SwitchCase *SC = SS->getSwitchCaseList(); SC && !HasDependentValue;
-   SC = SC->getNextSwitchCase()) {
+  SwitchCase *SC = SS->getSwitchCaseList();
+  if (!SC)
+Diag(SS->getBeginLoc(), diag::warn_no_cases_in_switch);
+  else if (isa(SC) && !SC->getNextSwitchCase())
+Diag(SC->getBeginLoc(), diag::warn_default_only_in_switch);
+
+  for (; SC && !HasDependentValue; SC = SC->getNextSwitchCase()) {
 
 if (DefaultStmt *DS = dyn_cast(SC)) {
   if (TheDefaultStmt) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8201,6 +8201,10 @@
 def warn_break_binds_to_switch : Warning<
   "'break' is bound to loop, GCC binds it to switch">,
   InGroup;
+def warn_no_cases_in_switch : Warning<
+  "switch has no default and case labels">, InGroup, DefaultIgnore;
+def warn_default_only_in_switch : Warning<
+  "switch contains only default label">, InGroup, DefaultIgnore;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -458,7 +458,6 @@
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
-def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
@@ -537,6 +536,7 @@
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool : DiagGroup<"switch-bool">;
+def SwitchDefault  : DiagGroup<"switch-default">;
 def SwitchEnum : DiagGroup<"switch-enum">;
 def Switch : DiagGroup<"switch">;
 def EnumCompareSwitch : DiagGroup<"enum-compare-switch">;
@@ -842,7 +842,7 @@
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting it here.
-def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool]>;
+def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, SwitchDefault]>;
 
 // Warnings that should be in clang-cl /w4.
 def : DiagGroup<"CL4", [All, Extra]>;

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 marked an inline comment as done.
xbolva00 added inline comments.



Comment at: lib/Sema/SemaStmt.cpp:869
+  SwitchCase *SC = SS->getSwitchCaseList();
+  if (!SC)
+Diag(SS->getBeginLoc(), diag::warn_empty_switch_body);

TODO:

if (!SC) -> switch has no default and case labels (empty body is misleading.. 
switch(x) x = b)

switch (x) {
   // warn_empty_switch_body
}


Repository:
  rC Clang

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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 planned changes to this revision.
xbolva00 added a comment.

I will work on this after we land https://reviews.llvm.org/D63139.


Repository:
  rC Clang

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

https://reviews.llvm.org/D63192



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D63192: [Diagnostics] Implement -Wswitch-default

[Dávid Bolvanský via Phabricator via cfe-commits]

xbolva00 created this revision.
xbolva00 added reviewers: hfinkel, lattner, rsmith.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

For MSVC compatibility, based on request in 
https://bugs.llvm.org/show_bug.cgi?id=4546.


Repository:
  rC Clang

https://reviews.llvm.org/D63192

Files:
  include/clang/Basic/DiagnosticGroups.td
  include/clang/Basic/DiagnosticSemaKinds.td
  lib/Sema/SemaStmt.cpp
  test/Sema/switch_default.c
  test/SemaCXX/switch_default.cpp

Index: test/SemaCXX/switch_default.cpp
===
--- test/SemaCXX/switch_default.cpp
+++ test/SemaCXX/switch_default.cpp
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wempty-body %s
+// RUN: %clang_cc1 -x c++ -fsyntax-only -verify -Wall %s
+
+void test(int x) {
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+break;
+  }
+
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+x++;
+  }
+
+  switch (x) {
+  default:; // expected-warning {{switch contains only default label}}
+  }
+
+  switch (x)
+  default: // expected-warning {{switch contains only default label}}
+break;
+
+  switch (x) {
+  default:
+return;
+  case 1:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+return;
+  }
+
+  switch (x) { // expected-warning {{switch statement has empty body}}
+  }
+}
Index: test/Sema/switch_default.c
===
--- test/Sema/switch_default.c
+++ test/Sema/switch_default.c
@@ -0,0 +1,40 @@
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wswitch-default %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wempty-body %s
+// RUN: %clang_cc1 -x c -fsyntax-only -verify -Wall %s
+
+void test(int x) {
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+break;
+  }
+
+  switch (x) {
+  default: // expected-warning {{switch contains only default label}}
+x++;
+  }
+
+  switch (x) {
+  default:; // expected-warning {{switch contains only default label}}
+  }
+
+  switch (x)
+  default: // expected-warning {{switch contains only default label}}
+break;
+
+  switch (x) {
+  default:
+return;
+  case 1:
+break;
+  }
+
+  switch (x) {
+  case 1:
+break;
+  default:
+return;
+  }
+
+  switch (x) { // expected-warning {{switch statement has empty body}}
+  }
+}
Index: lib/Sema/SemaStmt.cpp
===
--- lib/Sema/SemaStmt.cpp
+++ lib/Sema/SemaStmt.cpp
@@ -865,8 +865,13 @@
 
   bool CaseListIsErroneous = false;
 
-  for (SwitchCase *SC = SS->getSwitchCaseList(); SC && !HasDependentValue;
-   SC = SC->getNextSwitchCase()) {
+  SwitchCase *SC = SS->getSwitchCaseList();
+  if (!SC)
+Diag(SS->getBeginLoc(), diag::warn_empty_switch_body);
+  else if (isa(SC) && !SC->getNextSwitchCase())
+Diag(SC->getBeginLoc(), diag::warn_default_only_in_switch);
+
+  for (; SC && !HasDependentValue; SC = SC->getNextSwitchCase()) {
 
 if (DefaultStmt *DS = dyn_cast(SC)) {
   if (TheDefaultStmt) {
Index: include/clang/Basic/DiagnosticSemaKinds.td
===
--- include/clang/Basic/DiagnosticSemaKinds.td
+++ include/clang/Basic/DiagnosticSemaKinds.td
@@ -8185,6 +8185,8 @@
 def warn_break_binds_to_switch : Warning<
   "'break' is bound to loop, GCC binds it to switch">,
   InGroup;
+def warn_default_only_in_switch : Warning<
+  "switch contains only default label">, InGroup, DefaultIgnore;
 def err_default_not_in_switch : Error<
   "'default' statement not in switch statement">;
 def err_case_not_in_switch : Error<"'case' statement not in switch statement">;
Index: include/clang/Basic/DiagnosticGroups.td
===
--- include/clang/Basic/DiagnosticGroups.td
+++ include/clang/Basic/DiagnosticGroups.td
@@ -458,7 +458,6 @@
 def : DiagGroup<"sign-promo">;
 def SignCompare : DiagGroup<"sign-compare">;
 def : DiagGroup<"stack-protector">;
-def : DiagGroup<"switch-default">;
 def : DiagGroup<"synth">;
 def SizeofArrayArgument : DiagGroup<"sizeof-array-argument">;
 def SizeofArrayDecay : DiagGroup<"sizeof-array-decay">;
@@ -537,6 +536,7 @@
 def ObjCCStringFormat : DiagGroup<"cstring-format-directive">;
 def CoveredSwitchDefault : DiagGroup<"covered-switch-default">;
 def SwitchBool : DiagGroup<"switch-bool">;
+def SwitchDefault  : DiagGroup<"switch-default">;
 def SwitchEnum : DiagGroup<"switch-enum">;
 def Switch : DiagGroup<"switch">;
 def EnumCompareSwitch : DiagGroup<"enum-compare-switch">;
@@ -842,7 +842,7 @@
 // Note that putting warnings in -Wall will not disable them by default. If a
 // warning should be active _only_ when -Wall is passed in, mark it as
 // DefaultIgnore in addition to putting