diff mbox

[C++] Fix -Wlogical-not-parentheses (PR c++/62199)

Message ID 20140821154125.GS14320@redhat.com
State New
Headers show

Commit Message

Marek Polacek Aug. 21, 2014, 3:41 p.m. UTC 
On Wed, Aug 20, 2014 at 05:02:24PM -0400, Jason Merrill wrote:
> On 08/20/2014 04:02 PM, Marek Polacek wrote:
> >On Wed, Aug 20, 2014 at 02:36:21PM -0400, Jason Merrill wrote:
> >>Could we set current.lhs_type to TRUTH_NOT_EXPR when we see a ! rather than
> >>track nots in two separate local variables?
> >
> >Good point.  So like the following?
> 
> I was thinking to do away with parenthesized_not_lhs_warn as well, so
> instead of
> 
> >  bool parenthesized_not_lhs_warn
> >    = cp_lexer_next_token_is (parser->lexer, CPP_NOT);
> >
> >  /* Parse the first expression.  */
> >  current.lhs = cp_parser_cast_expression (parser, /*address_p=*/false,
> >                                           cast_p, decltype_p, pidk);
> >  current.lhs_type = ERROR_MARK;
> 
> we would set current.lhs_type to TRUTH_NOT_EXPR here if the first token is
> CPP_NOT, and
> 
> >      /* Extract another operand.  It may be the RHS of this expression
> >         or the LHS of a new, higher priority expression.  */
> >      rhs = cp_parser_simple_cast_expression (parser);
> >      rhs_type = ERROR_MARK;
> 
> here we would do the same thing for rhs_type.
> 
> >       cp_lexer_consume_token (parser->lexer);
> >+      if (cp_lexer_next_token_is (parser->lexer, CPP_NOT))
> >+	current.lhs_type = TRUTH_NOT_EXPR;
> ...
> > 	  current.lhs = rhs;
> >+	  parenthesized_not_lhs_warn = current.lhs_type == TRUTH_NOT_EXPR;
> > 	  current.lhs_type = rhs_type;
> 
> and then you don't need any changes in these places.
> 
> >       if (warn_logical_not_paren
> > 	  && parenthesized_not_lhs_warn)
> 
> And here you check current.lhs_type.

Oh, I see now.  Thanks, this is much better.
This change has an interesting effect on Wparentheses-25.C test; it
made it XPASS - I believe we now generate the correct warnings, the
same as C FE.  So I got rid of all those xfails in that test and it
now passes cleanly.

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

2014-08-21  Marek Polacek  <polacek@redhat.com>

	PR c++/62199
	* parser.c (cp_parser_binary_expression): Check each LHS if it's
	preceded with logical not.  Handle logical not of constants.

	* c-c++-common/pr62199.c: New test.
	* g++.dg/warn/Wparentheses-25.C: Remove XFAILs.


	Marek

Comments

Jason Merrill Aug. 21, 2014, 6:34 p.m. UTC | #1 
On 08/21/2014 11:41 AM, Marek Polacek wrote:
> +  current.lhs_type = cp_lexer_next_token_is (parser->lexer, CPP_NOT)
> +		     ? TRUTH_NOT_EXPR : ERROR_MARK;
...
> +      rhs_type = cp_lexer_next_token_is (parser->lexer, CPP_NOT)
> +		 ? TRUTH_NOT_EXPR : ERROR_MARK;

Again, this indentation needs parens to survive Emacs.

> +	  /* If the LHS was !CST, we have true/false now.  Convert it
> +	     to integer type, otherwise we wouldn't warn.  */
> +	  if (TREE_CODE (current.lhs) == INTEGER_CST)
> +	    lhs = convert (integer_type_node, current.lhs);

Hmm, why are we checking for BOOLEAN_TYPE on the lhs, again?  It seems 
to me that

bool b;
if (!b == 1)

is also probably an error.

Jason
diff mbox

Patch

diff --git gcc/cp/parser.c gcc/cp/parser.c
index 9053bfa..ddcbab9 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -8020,13 +8020,12 @@  cp_parser_binary_expression (cp_parser* parser, bool cast_p,
   enum tree_code rhs_type;
   enum cp_parser_prec new_prec, lookahead_prec;
   tree overload;
-  bool parenthesized_not_lhs_warn
-    = cp_lexer_next_token_is (parser->lexer, CPP_NOT);
 
   /* Parse the first expression.  */
+  current.lhs_type = cp_lexer_next_token_is (parser->lexer, CPP_NOT)
+		     ? TRUTH_NOT_EXPR : ERROR_MARK;
   current.lhs = cp_parser_cast_expression (parser, /*address_p=*/false,
 					   cast_p, decltype_p, pidk);
-  current.lhs_type = ERROR_MARK;
   current.prec = prec;
 
   if (cp_parser_error_occurred (parser))
@@ -8081,8 +8080,9 @@  cp_parser_binary_expression (cp_parser* parser, bool cast_p,
 
       /* Extract another operand.  It may be the RHS of this expression
 	 or the LHS of a new, higher priority expression.  */
+      rhs_type = cp_lexer_next_token_is (parser->lexer, CPP_NOT)
+		 ? TRUTH_NOT_EXPR : ERROR_MARK;
       rhs = cp_parser_simple_cast_expression (parser);
-      rhs_type = ERROR_MARK;
 
       /* Get another operator token.  Look up its precedence to avoid
 	 building a useless (immediately popped) stack entry for common
@@ -8125,9 +8125,18 @@  cp_parser_binary_expression (cp_parser* parser, bool cast_p,
 	c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node;
 
       if (warn_logical_not_paren
-	  && parenthesized_not_lhs_warn)
-	warn_logical_not_parentheses (current.loc, current.tree_type,
-				      TREE_OPERAND (current.lhs, 0), rhs);
+	  && current.lhs_type == TRUTH_NOT_EXPR)
+	{
+	  tree lhs;
+	  /* If the LHS was !CST, we have true/false now.  Convert it
+	     to integer type, otherwise we wouldn't warn.  */
+	  if (TREE_CODE (current.lhs) == INTEGER_CST)
+	    lhs = convert (integer_type_node, current.lhs);
+	  else
+	    lhs = TREE_OPERAND (current.lhs, 0);
+	  warn_logical_not_parentheses (current.loc, current.tree_type,
+					lhs, rhs);
+	}
 
       overload = NULL;
       /* ??? Currently we pass lhs_type == ERROR_MARK and rhs_type ==
diff --git gcc/testsuite/c-c++-common/pr62199.c gcc/testsuite/c-c++-common/pr62199.c
index e69de29..51078c8 100644
--- gcc/testsuite/c-c++-common/pr62199.c
+++ gcc/testsuite/c-c++-common/pr62199.c
@@ -0,0 +1,22 @@ 
+/* PR c++/62199 */
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-not-parentheses" } */
+
+int r;
+void
+foo (int a)
+{
+  r = a > 0 || !a >= 2; /* { dg-warning "19:logical not is only applied to the left hand side of comparison" } */
+  r = !a || a == 10;
+  r = !a && !a < 4; /* { dg-warning "16:logical not is only applied to the left hand side of comparison" } */
+  r = !a > 0 && a < 6; /* { dg-warning "10:logical not is only applied to the left hand side of comparison" } */
+  r = a + (!a < 12); /* { dg-warning "15:logical not is only applied to the left hand side of comparison" } */
+  r = a == 7 || !a < 12; /* { dg-warning "20:logical not is only applied to the left hand side of comparison" } */
+  r = (a == 7 * a > 0) || !a < 2; /* { dg-warning "30:logical not is only applied to the left hand side of comparison" } */
+  r = (1 > !a) || (!42 > a); /* { dg-warning "24:logical not is only applied to the left hand side of comparison" } */
+  r = (!5 > a); /* { dg-warning "11:logical not is only applied to the left hand side of comparison" } */
+  r = (!0 > a); /* { dg-warning "11:logical not is only applied to the left hand side of comparison" } */
+  r = (!-5 > a); /* { dg-warning "12:logical not is only applied to the left hand side of comparison" } */
+  r = (!(5 + 3) > a); /* { dg-warning "17:logical not is only applied to the left hand side of comparison" } */
+  r = (!(5 - a) > a); /* { dg-warning "17:logical not is only applied to the left hand side of comparison" } */
+}
diff --git gcc/testsuite/g++.dg/warn/Wparentheses-25.C gcc/testsuite/g++.dg/warn/Wparentheses-25.C
index ab00c25..d9951a4 100644
--- gcc/testsuite/g++.dg/warn/Wparentheses-25.C
+++ gcc/testsuite/g++.dg/warn/Wparentheses-25.C
@@ -8,7 +8,7 @@  int foo (int);
 int
 bar (int a, int b, int c)
 {
-  foo (!a & b); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!a & b); /* { dg-warning "parentheses" "correct warning" } */
   foo (!a & (b < c));
   foo (!a & (b > c));
   foo (!a & (b == c));
@@ -20,7 +20,7 @@  bar (int a, int b, int c)
   foo (!a & !b);
   foo (!(a & b));
   foo ((!a) & b);
-  foo (!a & 2); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!a & 2); /* { dg-warning "parentheses" "correct warning" } */
   foo (!a & (2 < c));
   foo (!a & (2 > c));
   foo (!a & (2 == c));
@@ -32,7 +32,7 @@  bar (int a, int b, int c)
   foo (!a & !2);
   foo (!(a & 2));
   foo ((!a) & 2);
-  foo (!1 & 2); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!1 & 2); /* { dg-warning "parentheses" "correct warning" } */
   foo (!1 & (2 < c));
   foo (!1 & (2 > c));
   foo (!1 & (2 == c));
@@ -44,7 +44,7 @@  bar (int a, int b, int c)
   foo (!1 & !2);
   foo (!(1 & 2));
 
-  foo (!a | b); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!a | b); /* { dg-warning "parentheses" "correct warning" } */
   foo (!a | (b < c));
   foo (!a | (b > c));
   foo (!a | (b == c));
@@ -56,7 +56,7 @@  bar (int a, int b, int c)
   foo (!a | !b);
   foo (!(a | b));
   foo ((!a) | b);
-  foo (!a | 2); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!a | 2); /* { dg-warning "parentheses" "correct warning" } */
   foo (!a | (2 < c));
   foo (!a | (2 > c));
   foo (!a | (2 == c));
@@ -68,7 +68,7 @@  bar (int a, int b, int c)
   foo (!a | !2);
   foo (!(a | 2));
   foo ((!a) | 2);
-  foo (!1 | 2); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!1 | 2); /* { dg-warning "parentheses" "correct warning" } */
   foo (!1 | (2 < c));
   foo (!1 | (2 > c));
   foo (!1 | (2 == c));
@@ -159,55 +159,55 @@  bar (int a, int b, int c)
 int
 baz (int a, int b, int c)
 {
-  foo (!a & (b << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b = c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & ~b);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (b | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & 2);       /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (c = 2)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & ~2);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a & (2 | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (c = 2)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & ~2);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 & (2 | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b = c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | ~b);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (b | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (c = 2)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | ~2);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!a | (2 | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 << c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 >> c));/* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 + c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 - c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (c = 2)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | ~2);      /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 & c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
-  foo (!1 | (2 | c)); /* { dg-warning "parentheses" "correct warning" { xfail *-*-* } } */
+  foo (!a & (b << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b = c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & ~b);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (b | c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & 2);       /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (c = 2)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & ~2);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a & (2 | c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (c = 2)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & ~2);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 & (2 | c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b = c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | ~b);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (b | c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (c = 2)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | ~2);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!a | (2 | c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 << c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 >> c));/* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 + c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 - c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (c = 2)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | ~2);      /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 & c)); /* { dg-warning "parentheses" "correct warning" } */
+  foo (!1 | (2 | c)); /* { dg-warning "parentheses" "correct warning" } */
   foo ((b << c) & !a);
   foo ((b >> c) & !a);
   foo ((b + c) & !a);