Patchwork [C/C++] Add -Wlogical-not-parentheses (PR c/49706)

login
register
mail settings
Submitter Marek Polacek
Date June 23, 2014, 12:09 p.m.
Message ID <20140623120923.GJ14420@redhat.com>
Download mbox | patch
Permalink /patch/362774/
State New
Headers show

Comments

Marek Polacek - June 23, 2014, 12:09 p.m.
On Sun, Jun 22, 2014 at 10:33:57PM +0200, Gerald Pfeifer wrote:
> On Mon, 2 Jun 2014, Marek Polacek wrote:
> > 	* c-typeck.c (parser_build_binary_op): Warn when logical not is used
> > 	on the left hand side operand of a comparison. 
> 
> This...
> 
> > +/* Warn about logical not used on the left hand side operand of a comparison.
> 
> ...and this...
> 
> > +  warning_at (location, OPT_Wlogical_not_parentheses,
> > +	      "logical not is only applied to the left hand side of "
> > +	      "comparison");
> 
> ...does not appear consistent with the actual warning.
> 
> Why does that warning say "is _ONLY_ applied to the left hand side"?
> 
> Based on the message, I naively assumed that the code should not warn
> about
> 
>   int same(int a, int b) {
>     return !a == !b;
>   }
> 
> alas this is not the case.  (Code like this occurs in Wine where
> bool types are emulated and !!a or a comparison like above ensure
> that those emulated bools are normalized to either 0 or 1.)
> 
> 
> I understand there is ambiguity in cases like
> 
>   return !a == b;
> 
> where the warning would be approriately worded and the programmer
> might have intended !(a == b).
> 
> 
> I do recommend to either omit "only" from the text of the warning
> or not warn for cases where ! occurs on both sides of the comparison
> (and keep the text as is).

I think the latter is better, incidentally, g++ doesn't warn either.
The following one liner makes cc1 behave as cc1plus.  Thanks for the
report.

Regtested/bootstrapped on x86_64.  Joseph, is this ok?

2014-06-23  Marek Polacek  <polacek@redhat.com>

	* c-typeck.c (parser_build_binary_op): Don't call
	warn_logical_not_parentheses if the RHS is TRUTH_NOT_EXPR.

	* c-c++-common/pr49706-2.c: New test.


	Marek
Joseph S. Myers - June 23, 2014, 2:35 p.m.
On Mon, 23 Jun 2014, Marek Polacek wrote:

> I think the latter is better, incidentally, g++ doesn't warn either.
> The following one liner makes cc1 behave as cc1plus.  Thanks for the
> report.
> 
> Regtested/bootstrapped on x86_64.  Joseph, is this ok?
> 
> 2014-06-23  Marek Polacek  <polacek@redhat.com>
> 
> 	* c-typeck.c (parser_build_binary_op): Don't call
> 	warn_logical_not_parentheses if the RHS is TRUTH_NOT_EXPR.
> 
> 	* c-c++-common/pr49706-2.c: New test.

OK.

Patch

diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 63bd65e..0764630 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3402,7 +3402,8 @@  parser_build_binary_op (location_t location, enum tree_code code,
 			   code1, arg1.value, code2, arg2.value);
 
   if (warn_logical_not_paren
-      && code1 == TRUTH_NOT_EXPR)
+      && code1 == TRUTH_NOT_EXPR
+      && code2 != TRUTH_NOT_EXPR)
     warn_logical_not_parentheses (location, code, arg1.value, arg2.value);
 
   /* Warn about comparisons against string literals, with the exception
diff --git gcc/testsuite/c-c++-common/pr49706-2.c gcc/testsuite/c-c++-common/pr49706-2.c
index e69de29..09cc9eb 100644
--- gcc/testsuite/c-c++-common/pr49706-2.c
+++ gcc/testsuite/c-c++-common/pr49706-2.c
@@ -0,0 +1,20 @@ 
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-not-parentheses" } */
+
+/* Test that we don't warn if both operands of the comparison
+   are negated.  */
+
+#ifndef __cplusplus
+#define bool _Bool
+#endif
+
+bool r;
+
+int
+same (int a, int b)
+{
+  r = !a == !b;
+  r = !!a == !!b;
+  r = !!a == !b;
+  r = !a == !!b;
+}