Hi, I have some comments/questions in-line: On 9/15/16, Marek Polacek <pola...@redhat.com> wrote: > Now that the C++ FE boolean in/decrement changes are in, I can move > forwards with this patch which implements a new warning named -Wbool-operation > (better names?) which warns about nonsensical code such as ~bool, ~(i == 10), > or > bool-- (in C). > > It could also warn for other operations such as * or / or <<, but I wasn't > sure about those, so this isn't included in this patch. > > Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk? > > 2016-09-15 Marek Polacek <pola...@redhat.com> > > PR c/77490 > * c.opt (Wbool-operation): New. > > * c-typeck.c (build_unary_op): Warn about bit not on expressions that > have boolean value. Warn about ++/-- on booleans. > > * typeck.c (cp_build_unary_op): Warn about bit not on expressions that > have boolean value. > > * doc/invoke.texi: Document -Wbool-operation. > > * c-c++-common/Wbool-operation-1.c: New test. > * gcc.dg/Wall.c: Use -Wno-bool-operation. > * gcc.dg/Wbool-operation-1.c: New test. > > diff --git gcc/c-family/c.opt gcc/c-family/c.opt > index c55c7c3..fb6f2d1 100644 > --- gcc/c-family/c.opt > +++ gcc/c-family/c.opt > @@ -315,6 +315,10 @@ Wbool-compare > C ObjC C++ ObjC++ Var(warn_bool_compare) Warning LangEnabledBy(C ObjC C++ > ObjC++,Wall) > Warn about boolean expression compared with an integer value different from > true/false. > > +Wbool-operation > +C ObjC C++ ObjC++ Var(warn_bool_op) Warning LangEnabledBy(C ObjC C++ > ObjC++,Wall) > +Warn about certain operations on boolean expressions. > + > Wframe-address > C ObjC C++ ObjC++ Var(warn_frame_address) Warning LangEnabledBy(C ObjC C++ > ObjC++,Wall) > Warn when __builtin_frame_address or __builtin_return_address is used > unsafely. > diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c > index 4dec397..c455d22 100644 > --- gcc/c/c-typeck.c > +++ gcc/c/c-typeck.c > @@ -4196,6 +4196,22 @@ build_unary_op (location_t location, enum tree_code > code, tree xarg, > || (typecode == VECTOR_TYPE > && !VECTOR_FLOAT_TYPE_P (TREE_TYPE (arg)))) > { > + tree e = arg; > + > + /* Warn if the expression has boolean value. */ > + while (TREE_CODE (e) == COMPOUND_EXPR) > + e = TREE_OPERAND (e, 1); > + > + if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE > + || truth_value_p (TREE_CODE (e))) > + && warning_at (location, OPT_Wbool_operation, > + "%<~%> on a boolean expression")) > + { > + gcc_rich_location richloc (location); > + richloc.add_fixit_insert_before (location, "!"); > + inform_at_rich_loc (&richloc, "did you mean to use logical " > + "not?"); > + } > if (!noconvert) > arg = default_conversion (arg); > } > @@ -4306,6 +4322,16 @@ build_unary_op (location_t location, enum tree_code > code, tree xarg, > "decrement of enumeration value is invalid in C++"); > } > > + if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE) > + { > + if (code == PREINCREMENT_EXPR || code == POSTINCREMENT_EXPR) > + warning_at (location, OPT_Wbool_operation, > + "increment of a boolean expression"); > + else > + warning_at (location, OPT_Wbool_operation, > + "decrement of a boolean expression"); > + } > + > /* Ensure the argument is fully folded inside any SAVE_EXPR. */ > arg = c_fully_fold (arg, false, NULL); > > diff --git gcc/cp/typeck.c gcc/cp/typeck.c > index c53a85a..9fc1a4b 100644 > --- gcc/cp/typeck.c > +++ gcc/cp/typeck.c > @@ -5853,7 +5853,16 @@ cp_build_unary_op (enum tree_code code, tree xarg, > bool noconvert, > arg, true))) > errstring = _("wrong type argument to bit-complement"); > else if (!noconvert && CP_INTEGRAL_TYPE_P (TREE_TYPE (arg))) > - arg = cp_perform_integral_promotions (arg, complain); > + { > + /* Warn if the expression has boolean value. */ > + location_t location = EXPR_LOC_OR_LOC (arg, input_location); > + if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE > + || truth_value_p (TREE_CODE (arg))) > + && warning_at (location, OPT_Wbool_operation, > + "%<~%> on a boolean expression")) > + inform (location, "did you mean to use logical not (%<!%>)?"); > + arg = cp_perform_integral_promotions (arg, complain); > + } > break; > > case ABS_EXPR: > diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi > index 8eb5eff..976e137 100644 > --- gcc/doc/invoke.texi > +++ gcc/doc/invoke.texi > @@ -256,8 +256,8 @@ Objective-C and Objective-C++ Dialects}. > -pedantic-errors @gol > -w -Wextra -Wall -Waddress -Waggregate-return @gol > -Wno-aggressive-loop-optimizations -Warray-bounds -Warray-bounds=@var{n} > @gol > --Wno-attributes -Wbool-compare -Wno-builtin-macro-redefined @gol > --Wc90-c99-compat -Wc99-c11-compat @gol > +-Wno-attributes -Wbool-compare -Wbool-operation @gol > +-Wno-builtin-macro-redefined -Wc90-c99-compat -Wc99-c11-compat @gol > -Wc++-compat -Wc++11-compat -Wc++14-compat -Wcast-align -Wcast-qual @gol > -Wchar-subscripts -Wclobbered -Wcomment -Wconditionally-supported @gol > -Wconversion -Wcoverage-mismatch -Wno-cpp -Wdangling-else -Wdate-time @gol > @@ -3654,6 +3654,7 @@ Options} and @ref{Objective-C and Objective-C++ > Dialect Options}. > @gccoptlist{-Waddress @gol > -Warray-bounds=1 @r{(only with} @option{-O2}@r{)} @gol > -Wbool-compare @gol > +-Wbool-operation @gol > -Wc++11-compat -Wc++14-compat@gol > -Wchar-subscripts @gol > -Wcomment @gol > @@ -4761,6 +4762,16 @@ if ((n > 1) == 2) @{ @dots{} @} > @end smallexample > This warning is enabled by @option{-Wall}. > > +@item -Wbool-operation > +@opindex Wno-bool-operation > +@opindex Wbool-operation > +Warn about suspicious operations on expressions of a boolean type. For > +instance, bitwise negation of a boolean is very likely a bug in the program. > +For C, this warning also warns about incrementing or decrementing a boolean, > +which rarely makes sense. > +
I'd also mention that C++ now warns about incrementing and decrementing anyways, even without this option. > +This warning is enabled by @option{-Wall}. > + > @item -Wduplicated-cond > @opindex Wno-duplicated-cond > @opindex Wduplicated-cond > diff --git gcc/testsuite/c-c++-common/Wbool-operation-1.c > gcc/testsuite/c-c++-common/Wbool-operation-1.c > index e69de29..287bd03 100644 > --- gcc/testsuite/c-c++-common/Wbool-operation-1.c > +++ gcc/testsuite/c-c++-common/Wbool-operation-1.c > @@ -0,0 +1,37 @@ > +/* PR c/77490 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wall" } */ > + > +#ifndef __cplusplus > +# define bool _Bool > +#endif > + > +typedef volatile bool T; > +typedef int __attribute__ ((vector_size (4 * sizeof (int)))) v4si; > +extern bool foo (void); > + > +int > +fn (bool b, bool b2, T b3, int n, v4si v) > +{ > + int r = 0; > + > + r += ~b; /* { dg-warning "on a boolean expression" } */ > + r += n + ~b; /* { dg-warning "on a boolean expression" } */ > + r += ~(n == 1); /* { dg-warning "on a boolean expression" } */ > + r += ~(n || 1); /* { dg-warning "on a boolean expression" } */ > + r += ~b == 1; /* { dg-warning "on a boolean expression" } */ > + r += ~(++n, n == 1); /* { dg-warning "on a boolean expression" } */ > + r += ~(++n, n > 1); /* { dg-warning "on a boolean expression" } */ > + r += ~(++n, n && 1); /* { dg-warning "on a boolean expression" } */ > + r += (++n, ~b); /* { dg-warning "on a boolean expression" } */ > + r += (++n, n + ~b); /* { dg-warning "on a boolean expression" } */ > + r += ~b3; /* { dg-warning "on a boolean expression" } */ > + r += ~foo (); /* { dg-warning "on a boolean expression" } */ > + r += ~(bool) !1; /* { dg-warning "on a boolean expression" } */ > + > + v = ~v; > + r += ~(int) b; > + r += -b; > + > + return r; > +} > diff --git gcc/testsuite/gcc.dg/Wall.c gcc/testsuite/gcc.dg/Wall.c > index 8984847..6c73533 100644 > --- gcc/testsuite/gcc.dg/Wall.c > +++ gcc/testsuite/gcc.dg/Wall.c > @@ -1,7 +1,7 @@ > /* PR 30437: Test -Wall > Don't change this without changing Wno-all.c as well. */ > /* { dg-do compile } */ > -/* { dg-options "-Wall" } */ > +/* { dg-options "-Wall -Wno-bool-operation" } */ > Why did -Wno-bool-operation need to be added here? > void foo(int a) > { > diff --git gcc/testsuite/gcc.dg/Wbool-operation-1.c > gcc/testsuite/gcc.dg/Wbool-operation-1.c > index e69de29..b24e763 100644 > --- gcc/testsuite/gcc.dg/Wbool-operation-1.c > +++ gcc/testsuite/gcc.dg/Wbool-operation-1.c > @@ -0,0 +1,16 @@ > +/* PR c/77490 */ > +/* { dg-do compile } */ > +/* { dg-options "-Wall" } */ > + > +int > +fn (_Bool b) > +{ > + int r = 0; > + > + r += b++; /* { dg-warning "increment of a boolean expression" } */ > + r += ++b; /* { dg-warning "increment of a boolean expression" } */ > + r += b--; /* { dg-warning "decrement of a boolean expression" } */ > + r += --b; /* { dg-warning "decrement of a boolean expression" } */ > + > + return r; > +} > > Marek > Anyways, thanks for your work on this; I hope it goes in soon! Eric