diff mbox

[C/C++] -Wlogical-not-parentheses tweaks (PR c/65120)

Message ID 20150220000326.GT1746@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 20, 2015, 12:03 a.m. UTC 
Hi!

As reported, !!x == y is quite common in the Linux kernel
and unlike the !x == y case it usually doesn't mean mistyped
!(x == y) or x != y.  clang++ apparently doesn't warn about that
either, and it doesn't warn even about the case where ! is applied
to a bool.  As the argument is already folded, it isn't easy to determine
those cases always, but I hope the following is sufficient until we switch
to late folding.

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

2015-02-19  Jakub Jelinek  <jakub@redhat.com>

	PR c/65120
	* c-typeck.c (parser_build_binary_op): Don't warn for
	!!x == y or !b == y where b is _Bool.

	* parser.c (cp_parser_binary_expression): Don't warn for
	!!x == y or !b == y where b is bool.

	* c-c++-common/pr49706.c: Adjust tests for not warning
	about !!x == y or !b == y where b is boolean, and add
	some further tests.
	* c-c++-common/pr62199-2.c: Likewise.


	Jakub

Comments

Marek Polacek Feb. 20, 2015, 3:35 p.m. UTC | #1 
On Fri, Feb 20, 2015 at 01:03:26AM +0100, Jakub Jelinek wrote:
> Hi!
> 
> As reported, !!x == y is quite common in the Linux kernel
> and unlike the !x == y case it usually doesn't mean mistyped
> !(x == y) or x != y.  clang++ apparently doesn't warn about that
> either, and it doesn't warn even about the case where ! is applied
> to a bool.

Note that first version of -Wlogical-not-parentheses didn't warn
when LHS had a boolean type, this has been changed later on.  I have
no strong preference either way.

> As the argument is already folded, it isn't easy to determine
> those cases always, but I hope the following is sufficient until we switch
> to late folding.
 
Yes, this means that we warn for

  return !(a != 0) == b;

but not for

  return !(a == 0) == b;

I think we can live with that for now.

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> 2015-02-19  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/65120
> 	* c-typeck.c (parser_build_binary_op): Don't warn for
> 	!!x == y or !b == y where b is _Bool.
> 
> 	* parser.c (cp_parser_binary_expression): Don't warn for
> 	!!x == y or !b == y where b is bool.
> 
> 	* c-c++-common/pr49706.c: Adjust tests for not warning
> 	about !!x == y or !b == y where b is boolean, and add
> 	some further tests.
> 	* c-c++-common/pr62199-2.c: Likewise.

The C part is ok.  Maybe we should also update the docs to reflect that
-Wlogical-not-parentheses does not warn if the RHS *or LHS* operand is of
a boolean type.  Thanks,

	Marek
Jakub Jelinek Feb. 20, 2015, 3:47 p.m. UTC | #2 
On Fri, Feb 20, 2015 at 04:35:05PM +0100, Marek Polacek wrote:
> Note that first version of -Wlogical-not-parentheses didn't warn
> when LHS had a boolean type, this has been changed later on.  I have
> no strong preference either way.
> 
> > As the argument is already folded, it isn't easy to determine
> > those cases always, but I hope the following is sufficient until we switch
> > to late folding.
>  
> Yes, this means that we warn for
> 
>   return !(a != 0) == b;
> 
> but not for
> 
>   return !(a == 0) == b;
> 
> I think we can live with that for now.

Well, for the !! case another option is, as we at least in the C++ FE
peek at the first token if it is !, peek another token if it is ! too.
Then we would warn for !(!a) == b and would not warn for !!a == b.
Guess that would be fine too.
For the ! of bool, if we want to detect that case (have done that primarily
because clang++ does that (clang doesn't support
-Wlogical-not-parentheses)), for C because of the conversion to int it is
still more likely we catch it, but for C++ we'd need to parse the expression
twice or do similar uglities.
For everything the answer is of course less folding early, but it will take
some time.

> The C part is ok.  Maybe we should also update the docs to reflect that
> -Wlogical-not-parentheses does not warn if the RHS *or LHS* operand is of
> a boolean type.  Thanks,

RHS operand or operand of ! on the LHS to be precise, though that is not
what is implemented for C++ right now, e.g. !(a > 20) == 0 shouldn't warn
in C++ because a > 20 is bool, but it is really hard after the folding to
find out what was the original ! operand.
Though of course, it would be weird if we don't warn for !(a > 20) == 0
for C (where a > 20 is not _Bool) but do warn for C++ (where it is bool).

If preferred, I can do just the !! case that in theory should be reliably
detected, and warn for everything else.

	Jakub
Jason Merrill Feb. 27, 2015, 10:29 p.m. UTC | #3 
On 02/19/2015 07:03 PM, Jakub Jelinek wrote:
> +	  /* Avoid warning for !!b == y where b is boolean.  */
> +	  && (!DECL_P (current.lhs)
> +	      || TREE_TYPE (current.lhs) == NULL_TREE
> +	      || TREE_CODE (TREE_TYPE (current.lhs)) != BOOLEAN_TYPE))

There's something wrong here.  If the type is null, trying to check its 
TREE_CODE will SEGV.

Jason
Jakub Jelinek March 9, 2015, 2:34 p.m. UTC | #4 
On Fri, Feb 27, 2015 at 05:29:47PM -0500, Jason Merrill wrote:
> On 02/19/2015 07:03 PM, Jakub Jelinek wrote:
> >+	  /* Avoid warning for !!b == y where b is boolean.  */
> >+	  && (!DECL_P (current.lhs)
> >+	      || TREE_TYPE (current.lhs) == NULL_TREE
> >+	      || TREE_CODE (TREE_TYPE (current.lhs)) != BOOLEAN_TYPE))
> 
> There's something wrong here.  If the type is null, trying to check its
> TREE_CODE will SEGV.

If the type is NULL, then it will just call warn_logical_not_parentheses
and won't check TREE_CODE.  Only when it is non-NULL, it will check
TREE_CODE and if it is not BOOLEAN_TYPE, will call
warn_logical_not_parentheses.

	Jakub
Jason Merrill March 9, 2015, 4:13 p.m. UTC | #5 
On 03/09/2015 10:34 AM, Jakub Jelinek wrote:
> On Fri, Feb 27, 2015 at 05:29:47PM -0500, Jason Merrill wrote:
>> On 02/19/2015 07:03 PM, Jakub Jelinek wrote:
>>> +	  /* Avoid warning for !!b == y where b is boolean.  */
>>> +	  && (!DECL_P (current.lhs)
>>> +	      || TREE_TYPE (current.lhs) == NULL_TREE
>>> +	      || TREE_CODE (TREE_TYPE (current.lhs)) != BOOLEAN_TYPE))
>>
>> There's something wrong here.  If the type is null, trying to check its
>> TREE_CODE will SEGV.
>
> If the type is NULL, then it will just call warn_logical_not_parentheses
> and won't check TREE_CODE.  Only when it is non-NULL, it will check
> TREE_CODE and if it is not BOOLEAN_TYPE, will call
> warn_logical_not_parentheses.

You're right, of course.  I guess I was reading the || as an &&.  The 
patch is OK.

Jason
Gerald Pfeifer April 10, 2015, 7:40 p.m. UTC | #6 
On Fri, 20 Feb 2015, Jakub Jelinek wrote:
> As reported, !!x == y is quite common in the Linux kernel and unlike the 
> !x == y case it usually doesn't mean mistyped !(x == y) or x != y.  
> clang++ apparently doesn't warn about that either, and it doesn't warn 
> even about the case where ! is applied to a bool.  As the argument is 
> already folded, it isn't easy to determine those cases always, but I 
> hope the following is sufficient until we switch to late folding.

For what it's worth, with this change you eliminated all except
for four false positives in the entire Wine codebase.  And those
four are really only one (times four) and can be avoided easily
apparently.

So, good stuff to begin with, and with these improvements even
more useful.

Thank you!

Gerald
diff mbox

Patch

--- gcc/c/c-typeck.c.jj	2015-02-09 22:19:53.000000000 +0100
+++ gcc/c/c-typeck.c	2015-02-19 22:35:38.522781551 +0100
@@ -3460,8 +3460,34 @@  parser_build_binary_op (location_t locat
 
   if (warn_logical_not_paren
       && code1 == TRUTH_NOT_EXPR
-      && code2 != TRUTH_NOT_EXPR)
-    warn_logical_not_parentheses (location, code, arg2.value);
+      && code2 != TRUTH_NOT_EXPR
+      /* Avoid warning for !!x == y.  */
+      && (TREE_CODE (arg1.value) != NE_EXPR
+	  || !integer_zerop (TREE_OPERAND (arg1.value, 1))))
+    {
+      /* Avoid warning for !b == y where b has _Bool type.  */
+      tree t = integer_zero_node;
+      if (TREE_CODE (arg1.value) == EQ_EXPR
+	  && integer_zerop (TREE_OPERAND (arg1.value, 1))
+	  && TREE_TYPE (TREE_OPERAND (arg1.value, 0)) == integer_type_node)
+	{
+	  t = TREE_OPERAND (arg1.value, 0);
+	  do
+	    {
+	      if (TREE_TYPE (t) != integer_type_node)
+		break;
+	      if (TREE_CODE (t) == C_MAYBE_CONST_EXPR)
+		t = C_MAYBE_CONST_EXPR_EXPR (t);
+	      else if (CONVERT_EXPR_P (t))
+		t = TREE_OPERAND (t, 0);
+	      else
+		break;
+	    }
+	  while (1);
+	}
+      if (TREE_CODE (TREE_TYPE (t)) != BOOLEAN_TYPE)
+	warn_logical_not_parentheses (location, code, arg2.value);
+    }
 
   /* Warn about comparisons against string literals, with the exception
      of testing for equality or inequality of a string literal with NULL.  */
--- gcc/cp/parser.c.jj	2015-02-14 00:19:49.000000000 +0100
+++ gcc/cp/parser.c	2015-02-19 22:04:19.059604707 +0100
@@ -8270,7 +8270,20 @@  cp_parser_binary_expression (cp_parser*
 	c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node;
 
       if (warn_logical_not_paren
-	  && current.lhs_type == TRUTH_NOT_EXPR)
+	  && current.lhs_type == TRUTH_NOT_EXPR
+	  /* Avoid warning for !!x == y.  */
+	  && (TREE_CODE (current.lhs) != NE_EXPR
+	      || !integer_zerop (TREE_OPERAND (current.lhs, 1)))
+	  && (TREE_CODE (current.lhs) != TRUTH_NOT_EXPR
+	      || (TREE_CODE (TREE_OPERAND (current.lhs, 0)) != TRUTH_NOT_EXPR
+		  /* Avoid warning for !b == y where b is boolean.  */
+		  && (TREE_TYPE (TREE_OPERAND (current.lhs, 0)) == NULL_TREE
+		      || (TREE_CODE (TREE_TYPE (TREE_OPERAND (current.lhs, 0)))
+			  != BOOLEAN_TYPE))))
+	  /* Avoid warning for !!b == y where b is boolean.  */
+	  && (!DECL_P (current.lhs)
+	      || TREE_TYPE (current.lhs) == NULL_TREE
+	      || TREE_CODE (TREE_TYPE (current.lhs)) != BOOLEAN_TYPE))
 	warn_logical_not_parentheses (current.loc, current.tree_type, rhs);
 
       overload = NULL;
--- gcc/testsuite/c-c++-common/pr49706.c.jj	2014-06-06 09:19:20.000000000 +0200
+++ gcc/testsuite/c-c++-common/pr49706.c	2015-02-19 20:53:27.466594656 +0100
@@ -12,10 +12,13 @@  extern bool foo_b (void);
 extern int foo_i (void);
 
 #ifdef __cplusplus
-template <class T, class U> bool f1(T t, U u) { return (!t == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 15 } */
-template <class T, class U> bool f2(T t, U u) { return ((!t) == u); }
-template <class T, class U> bool f3(T t, U u) { return (!g(t) == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 17 } */
-template <class T, class U> bool f4(T t, U u) { return ((!g(t)) == u); }
+template <class T, class U> bool tfn1(T t, U u) { return (!t == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 15 } */
+template <class T, class U> bool tfn2(T t, U u) { return ((!t) == u); }
+template <class T, class U> bool tfn3(T t, U u) { return (!g(t) == u); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 17 } */
+template <class T, class U> bool tfn4(T t, U u) { return ((!g(t)) == u); }
+template <class T, class U> bool tfn5(T t, U u) { return (!!t == u); } /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+template <class T, class U> bool tfn6(T t, U u) { return (!!g(t) == u); } /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+template <int N> bool tfn7(int i1, int i2) { return (!i1 == i2); } /* { dg-warning "logical not is only applied to the left hand side of comparison" "" { target c++ } 21 } */
 #endif
 
 void
@@ -58,23 +61,42 @@  fn1 (int i1, int i2, bool b1, bool b2)
   b = !b1 <= b2;
   b = !b1 >= b2;
 
+  b = !b1 == i2;
+  b = !b1 != i2;
+  b = !b1 < i2;
+  b = !b1 > i2;
+  b = !b1 <= i2;
+  b = !b1 >= i2;
+
   b = !foo_i () == i1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
   b = (!foo_i ()) == i1;
   b = !foo_b () == b1;
 
-  b = !!i1 == i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!i1 != i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!i1 < i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!i1 > i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!i1 <= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!i1 >= i2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  b = !!foo_i () == i1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 == i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 != i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 < i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 > i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 <= i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 >= i2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!foo_i () == i1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+
+  b = !!b1 == i2;
+  b = !!b1 != i2;
+  b = !!b1 < i2;
+  b = !!b1 > i2;
+  b = !!b1 <= i2;
+  b = !!b1 >= i2;
 
   /* Be careful here.  */
   b = (i1 == 0) != 0;
   b = (i1 == 0) == 0;
   b = (i1 != 0) != 0;
   b = (i1 != 0) == 0;
+
+  b = !5 == 4; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !!5 == 4; /* { dg-bogus "logical not is only applied to the left hand side of comparison" "" { xfail *-*-* } } */
+  b = !1 == 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !!1 == 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" "" { xfail *-*-* } } */
 }
 
 void
@@ -100,3 +122,44 @@  fn2 (enum E e)
   b = (!foo_e ()) == A;
   b = (!foo_e ()) == foo_e ();
 }
+
+void
+fn3 (int i1, float f2)
+{
+  b = !i1 == f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !i1 != f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !i1 < f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !i1 > f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !i1 <= f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !i1 >= f2; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+
+  b = i1 == f2;
+  b = i1 != f2;
+  b = i1 < f2;
+  b = i1 > f2;
+  b = i1 <= f2;
+  b = i1 >= f2;
+
+  /* Parens suppress the warning.  */
+  b = (!i1) == f2;
+  b = (!i1) != f2;
+  b = (!i1) < f2;
+  b = (!i1) > f2;
+  b = (!i1) <= f2;
+  b = (!i1) >= f2;
+
+  /* ...but not these parens.  */
+  b = (!i1 == f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = (!i1 != f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = (!i1 < f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = (!i1 > f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = (!i1 <= f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = (!i1 >= f2); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+
+  b = !!i1 == f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 != f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 < f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 > f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 <= f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  b = !!i1 >= f2; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+}
--- gcc/testsuite/c-c++-common/pr62199-2.c.jj	2014-09-01 09:43:39.000000000 +0200
+++ gcc/testsuite/c-c++-common/pr62199-2.c	2015-02-20 00:57:07.362564155 +0100
@@ -11,10 +11,10 @@  bool r;
 void
 foo (bool b)
 {
-  r = !b == 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  r = !b != 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  r = !b > 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  r = !b >= 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  r = !b < 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
-  r = !b <= 1; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  r = !b == 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  r = !b != 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  r = !b > 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  r = !b >= 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  r = !b < 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
+  r = !b <= 1; /* { dg-bogus "logical not is only applied to the left hand side of comparison" } */
 }