Patchwork [C] Warn if switch has boolean value (PR c/60439)

login
register
mail settings
Submitter Marek Polacek
Date April 18, 2014, 5:30 a.m.
Message ID <20140418053021.GH20332@redhat.com>
Download mbox | patch
Permalink /patch/340218/
State New
Headers show

Comments

Marek Polacek - April 18, 2014, 5:30 a.m.
This patch implements a new warning that warns when controlling
expression of a switch has boolean value.  (Intentionally I don't
warn if the controlling expression is (un)signed:1 bit-field.)
I guess the question is if this should be enabled by default or
deserves some new warning option.  Since clang does the former,
I did it too and currently this warning is enabled by default.

Regtested/bootstrapped on x86_64-linux, ok for trunk?

2014-04-17  Marek Polacek  <polacek@redhat.com>

	PR c/60439
c/
	* c-typeck.c (c_start_case): Warn if switch condition has boolean
	value.
testsuite/
	* gcc.dg/pr60439.c: New test.


	Marek
Marc Glisse - April 18, 2014, 5:49 a.m.
On Fri, 18 Apr 2014, Marek Polacek wrote:

> This patch implements a new warning that warns when controlling
> expression of a switch has boolean value.  (Intentionally I don't
> warn if the controlling expression is (un)signed:1 bit-field.)
> I guess the question is if this should be enabled by default or
> deserves some new warning option.  Since clang does the former,
> I did it too and currently this warning is enabled by default.

It can be enabled by -Wsome-name which is itself enabled by default but
at least gives the possibility to use -Wno-some-name, -Werror=some-name,
etc. No? I believe Manuel insists regularly that no new warning should
use 0 (and old ones should progressively lose it).
Marek Polacek - April 18, 2014, 6 a.m.
On Fri, Apr 18, 2014 at 07:49:22AM +0200, Marc Glisse wrote:
> On Fri, 18 Apr 2014, Marek Polacek wrote:
> 
> >This patch implements a new warning that warns when controlling
> >expression of a switch has boolean value.  (Intentionally I don't
> >warn if the controlling expression is (un)signed:1 bit-field.)
> >I guess the question is if this should be enabled by default or
> >deserves some new warning option.  Since clang does the former,
> >I did it too and currently this warning is enabled by default.
> 
> It can be enabled by -Wsome-name which is itself enabled by default but
> at least gives the possibility to use -Wno-some-name, -Werror=some-name,
> etc. No? I believe Manuel insists regularly that no new warning should
> use 0 (and old ones should progressively lose it).

Yes, that's the other possibility and exactly what I wanted to
discuss.  I think I'll prepare another version with -Wswitch-bool (and
documentation).

	Marek
Jakub Jelinek - April 18, 2014, 7:11 a.m.
On Fri, Apr 18, 2014 at 07:49:22AM +0200, Marc Glisse wrote:
> On Fri, 18 Apr 2014, Marek Polacek wrote:
> 
> >This patch implements a new warning that warns when controlling
> >expression of a switch has boolean value.  (Intentionally I don't
> >warn if the controlling expression is (un)signed:1 bit-field.)
> >I guess the question is if this should be enabled by default or
> >deserves some new warning option.  Since clang does the former,
> >I did it too and currently this warning is enabled by default.
> 
> It can be enabled by -Wsome-name which is itself enabled by default but
> at least gives the possibility to use -Wno-some-name, -Werror=some-name,
> etc. No? I believe Manuel insists regularly that no new warning should
> use 0 (and old ones should progressively lose it).

Yeah, completely agreed.  It can be enabled by default, but it should still be
constorlled by a warning switch.

	Jakub
Steven Bosscher - April 18, 2014, 11:20 a.m.
On Fri, Apr 18, 2014 at 7:30 AM, Marek Polacek wrote:
> +         if (TREE_CODE (type) == BOOLEAN_TYPE
> +             || exp_code == TRUTH_ANDIF_EXPR
> +             || exp_code == TRUTH_AND_EXPR
> +             || exp_code == TRUTH_ORIF_EXPR
> +             || exp_code == TRUTH_OR_EXPR
> +             || exp_code == TRUTH_XOR_EXPR
> +             || exp_code == TRUTH_NOT_EXPR
> +             || exp_code == EQ_EXPR
> +             || exp_code == NE_EXPR
> +             || exp_code == LE_EXPR
> +             || exp_code == GE_EXPR
> +             || exp_code == LT_EXPR
> +             || exp_code == GT_EXPR)

Is there a TREE_CODE_CLASS or a #define for this?

Ciao!
Steven
Marc Glisse - April 18, 2014, noon
On Fri, 18 Apr 2014, Steven Bosscher wrote:

> On Fri, Apr 18, 2014 at 7:30 AM, Marek Polacek wrote:
>> +         if (TREE_CODE (type) == BOOLEAN_TYPE
>> +             || exp_code == TRUTH_ANDIF_EXPR
>> +             || exp_code == TRUTH_AND_EXPR
>> +             || exp_code == TRUTH_ORIF_EXPR
>> +             || exp_code == TRUTH_OR_EXPR
>> +             || exp_code == TRUTH_XOR_EXPR
>> +             || exp_code == TRUTH_NOT_EXPR
>> +             || exp_code == EQ_EXPR
>> +             || exp_code == NE_EXPR
>> +             || exp_code == LE_EXPR
>> +             || exp_code == GE_EXPR
>> +             || exp_code == LT_EXPR
>> +             || exp_code == GT_EXPR)
>
> Is there a TREE_CODE_CLASS or a #define for this?

truth_value_p

Patch

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 65aad45..91b1109 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -9344,6 +9344,28 @@  c_start_case (location_t switch_loc,
       else
 	{
 	  tree type = TYPE_MAIN_VARIANT (orig_type);
+	  tree e = exp;
+	  enum tree_code exp_code;
+
+	  while (TREE_CODE (e) == COMPOUND_EXPR)
+	    e = TREE_OPERAND (e, 1);
+	  exp_code = TREE_CODE (e);
+
+	  if (TREE_CODE (type) == BOOLEAN_TYPE
+	      || exp_code == TRUTH_ANDIF_EXPR
+	      || exp_code == TRUTH_AND_EXPR
+	      || exp_code == TRUTH_ORIF_EXPR
+	      || exp_code == TRUTH_OR_EXPR
+	      || exp_code == TRUTH_XOR_EXPR
+	      || exp_code == TRUTH_NOT_EXPR
+	      || exp_code == EQ_EXPR
+	      || exp_code == NE_EXPR
+	      || exp_code == LE_EXPR
+	      || exp_code == GE_EXPR
+	      || exp_code == LT_EXPR
+	      || exp_code == GT_EXPR)
+	    warning_at (switch_cond_loc, 0,
+			"switch condition has boolean value");
 
 	  if (!in_system_header_at (input_location)
 	      && (type == long_integer_type_node
diff --git gcc/testsuite/gcc.dg/pr60439.c gcc/testsuite/gcc.dg/pr60439.c
index e69de29..26e7c25 100644
--- gcc/testsuite/gcc.dg/pr60439.c
+++ gcc/testsuite/gcc.dg/pr60439.c
@@ -0,0 +1,112 @@ 
+/* PR c/60439 */
+/* { dg-do compile } */
+
+typedef _Bool bool;
+extern _Bool foo (void);
+
+void
+f1 (const _Bool b)
+{
+  switch (b) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f2 (int a, int b)
+{
+  switch (a && b) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch ((bool) (a && b)) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch ((a && b) || a) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f3 (int a)
+{
+  switch (!!a) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (!a) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f4 (void)
+{
+  switch (foo ()) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f5 (int a)
+{
+  switch (a == 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a != 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a > 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a < 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a <= 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a >= 3) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (foo (), foo (), a >= 42) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (a == 3, a & 4, a ^ 5, a)
+    case 1:
+      break;
+}
+
+void
+f6 (bool b)
+{
+  switch (b) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (!b) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+  switch (b++) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f7 (void)
+{
+  bool b;
+  switch (b = 1) /* { dg-warning "switch condition has boolean value" } */
+    case 1:
+      break;
+}
+
+void
+f8 (int i)
+{
+  switch (i)
+    case 0:
+      break;
+  switch ((unsigned int) i)
+    case 0:
+      break;
+  switch ((bool) i) /* { dg-warning "switch condition has boolean value" } */
+    case 0:
+      break;
+}