| Message ID | 20140418053021.GH20332@redhat.com |
|---|---|
| State | New |
| Headers | show |
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).
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
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
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
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
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; +}