diff mbox

C/C++ PATCH to implement -Wbool-operation (PR c/77490)

Message ID 20160920171331.GG19950@redhat.com
State New
Headers show

Commit Message

Marek Polacek Sept. 20, 2016, 5:13 p.m. UTC
On Mon, Sep 19, 2016 at 02:41:04PM -0400, Jason Merrill wrote:
> On 09/16/2016 05:01 AM, Marek Polacek wrote:
> > @@ -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);
> 
> Let's move this variable to the beginning of the function; hopefully it will
> become a parameter soon.
 
Done.

> > +	  if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
> > +	       || truth_value_p (TREE_CODE (arg)))
> 
> You shouldn't need to check truth_value_p; in C++ all truth operations have
> type bool.

Oops, force of habit...

> > +	      && warning_at (location, OPT_Wbool_operation,
> > +			     "%<~%> on a boolean expression"))
> 
> And let's refer to "expression of type bool" rather than "boolean
> expression".

Adjusted (and in the C FE too).

Bootstrapped/regtested on x86_64-linux and ppc64-linux, ok for trunk?

2016-09-20  Marek Polacek  <polacek@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/Wbool-operation-1.c: New test.


	Marek

Comments

Jason Merrill Sept. 21, 2016, 3:37 p.m. UTC | #1
On 09/20/2016 01:13 PM, Marek Polacek wrote:
> On Mon, Sep 19, 2016 at 02:41:04PM -0400, Jason Merrill wrote:
>> On 09/16/2016 05:01 AM, Marek Polacek wrote:
>>> @@ -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);
>>
>> Let's move this variable to the beginning of the function; hopefully it will
>> become a parameter soon.
>
> Done.
>
>>> +	  if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
>>> +	       || truth_value_p (TREE_CODE (arg)))
>>
>> You shouldn't need to check truth_value_p; in C++ all truth operations have
>> type bool.
>
> Oops, force of habit...
>
>>> +	      && warning_at (location, OPT_Wbool_operation,
>>> +			     "%<~%> on a boolean expression"))
>>
>> And let's refer to "expression of type bool" rather than "boolean
>> expression".
>
> Adjusted (and in the C FE too).

Hmm, I'm not sure that change is right for C.  But the C++ hunk is OK.

Jason
Marek Polacek Sept. 21, 2016, 3:40 p.m. UTC | #2
On Wed, Sep 21, 2016 at 11:37:32AM -0400, Jason Merrill wrote:
> On 09/20/2016 01:13 PM, Marek Polacek wrote:
> > On Mon, Sep 19, 2016 at 02:41:04PM -0400, Jason Merrill wrote:
> > > On 09/16/2016 05:01 AM, Marek Polacek wrote:
> > > > @@ -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);
> > > 
> > > Let's move this variable to the beginning of the function; hopefully it will
> > > become a parameter soon.
> > 
> > Done.
> > 
> > > > +	  if ((TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
> > > > +	       || truth_value_p (TREE_CODE (arg)))
> > > 
> > > You shouldn't need to check truth_value_p; in C++ all truth operations have
> > > type bool.
> > 
> > Oops, force of habit...
> > 
> > > > +	      && warning_at (location, OPT_Wbool_operation,
> > > > +			     "%<~%> on a boolean expression"))
> > > 
> > > And let's refer to "expression of type bool" rather than "boolean
> > > expression".
> > 
> > Adjusted (and in the C FE too).
> 
> Hmm, I'm not sure that change is right for C.  But the C++ hunk is OK.

Thanks.  Joseph, how about the C part?

	Marek
Joseph Myers Sept. 22, 2016, 5:24 p.m. UTC | #3
On Wed, 21 Sep 2016, Marek Polacek wrote:

> > > > And let's refer to "expression of type bool" rather than "boolean
> > > > expression".
> > > 
> > > Adjusted (and in the C FE too).
> > 
> > Hmm, I'm not sure that change is right for C.  But the C++ hunk is OK.
> 
> Thanks.  Joseph, how about the C part?

You shouldn't refer to "type bool" for C.  If it's the result of a boolean 
operation, the type is int; an expression has type _Bool only if cast to 
that type or if it's a variable of that type.
diff mbox

Patch

diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 6cf915d..0e8d68a 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 059ad1f..4006b1a 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 an expression of type bool"))
+	    {
+	      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 an expression of type bool");
+	  else
+	    warning_at (location, OPT_Wbool_operation,
+			"decrement of an expression of type bool");
+	}
+
       /* 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..dee17b3 100644
--- gcc/cp/typeck.c
+++ gcc/cp/typeck.c
@@ -5792,6 +5792,7 @@  cp_build_unary_op (enum tree_code code, tree xarg, bool noconvert,
 {
   /* No default_conversion here.  It causes trouble for ADDR_EXPR.  */
   tree arg = xarg;
+  location_t location = EXPR_LOC_OR_LOC (arg, input_location);
   tree argtype = 0;
   const char *errstring = NULL;
   tree val;
@@ -5853,7 +5854,14 @@  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.  */
+	  if (TREE_CODE (TREE_TYPE (arg)) == BOOLEAN_TYPE
+	      && warning_at (location, OPT_Wbool_operation,
+			     "%<~%> on an expression of type bool"))
+	    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 3c27283..6e8218c 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
@@ -4762,6 +4763,17 @@  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.  (In C++, decrementing a boolean is always invalid.
+Incrementing a boolean is invalid in C++1z, and deprecated otherwise.)
+
+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..e81e89d 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 an expression of type bool" } */
+  r += n + ~b; /* { dg-warning "on an expression of type bool" } */
+  r += ~(n == 1); /* { dg-warning "on an expression of type bool" } */
+  r += ~(n || 1); /* { dg-warning "on an expression of type bool" } */
+  r += ~b == 1; /* { dg-warning "on an expression of type bool" } */
+  r += ~(++n, n == 1); /* { dg-warning "on an expression of type bool" } */
+  r += ~(++n, n > 1); /* { dg-warning "on an expression of type bool" } */
+  r += ~(++n, n && 1); /* { dg-warning "on an expression of type bool" } */
+  r += (++n, ~b); /* { dg-warning "on an expression of type bool" } */
+  r += (++n, n + ~b); /* { dg-warning "on an expression of type bool" } */
+  r += ~b3; /* { dg-warning "on an expression of type bool" } */
+  r += ~foo (); /* { dg-warning "on an expression of type bool" } */
+  r += ~(bool) !1; /* { dg-warning "on an expression of type bool" } */
+
+  v = ~v;
+  r += ~(int) b;
+  r += -b;
+
+  return r;
+}
diff --git gcc/testsuite/gcc.dg/Wbool-operation-1.c gcc/testsuite/gcc.dg/Wbool-operation-1.c
index e69de29..f3acc54 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 an expression of type bool" } */
+  r += ++b; /* { dg-warning "increment of an expression of type bool" } */
+  r += b--; /* { dg-warning "decrement of an expression of type bool" } */
+  r += --b; /* { dg-warning "decrement of an expression of type bool" } */
+
+  return r;
+}