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

login
register
mail settings
Submitter Marek Polacek
Date June 2, 2014, 6:50 a.m.
Message ID <20140602065052.GE31671@redhat.com>
Download mbox | patch
Permalink /patch/354771/
State New
Headers show

Comments

Marek Polacek - June 2, 2014, 6:50 a.m.
PR61271 showed that this warning, recently added to clang, is quite
useful and detected several reckless cases in the GCC codebase.
This patch adds this warning even for GCC.  Due to PR61271,
it's not a part of -Wall, that would break the bootstrap, but
after that is fixed, I think we want this warning be a part of
-Wall or -Wextra.

It's possible to suppress the warning by enclosing the LHS
in parentheses.  This proved to be hard to achieve in the C++
FE, so I had to go all the way up to parser...  Jason, does the
"bool parenthesized_not_lhs_warn" line looks reasonable?

Also for C++, I think we don't want this warning to trigger when the
operator (==, !=, >, <, <=, >=) is overloaded.  But I admit
I haven't even tried that, and I don't know how to detect overloaded
operators except DECL_OVERLOADED_OPERATOR_P.

Regtested/bootstrapped on x86_64-unknown-linux-gnu, ok for trunk?

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

	PR c/49706
	* doc/invoke.texi: Document -Wlogical-not-parentheses.
c-family/
	* c-common.c (warn_logical_not_parentheses): New function.
	* c-common.h (warn_logical_not_parentheses): Declare.
	* c.opt (Wlogical-not-parentheses): New option.
c/
	* c-typeck.c (parser_build_binary_op): Warn when logical not is used
	on the left hand side operand of a comparison. 
cp/
	* parser.c (cp_parser_binary_expression): Warn when logical not is
	used on the left hand side operand of a comparison.
testsuite/
	* c-c++-common/pr49706.c: New test.


	Marek
Jason Merrill - June 2, 2014, 2:08 p.m.
On 06/02/2014 02:50 AM, Marek Polacek wrote:
> +      && TREE_CODE (arg1.value) == EQ_EXPR)
...
> +	  && TREE_CODE (current.lhs) == EQ_EXPR

It seems like your version only warns about ==, while the clang version 
warns about all comparisons.

> +	  && (complain_flags (decltype_p) & tf_warning)

Let's not call complain_flags multiple times in the function.  In fact 
this will always be true, so you could just drop it.

> Also for C++, I think we don't want this warning to trigger when the
> operator (==, !=, >, <, <=, >=) is overloaded.  But I admit
> I haven't even tried that, and I don't know how to detect overloaded
> operators except DECL_OVERLOADED_OPERATOR_P.

Overloaded operators have the same precedence, so I think it's 
appropriate to give the warning whether or not the operators are overloaded.

Jason
Gerald Pfeifer - June 22, 2014, 8:33 p.m.
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).

Gerald

Patch

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 6ec14fc..650afaf 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1722,6 +1722,25 @@  warn_logical_operator (location_t location, enum tree_code code, tree type,
     }
 }
 
+/* Warn about logical not used on the left hand side operand of a comparison.
+   This function assumes that the LHS is inside of TRUTH_NOT_EXPR.
+   Do not warn if the LHS or RHS is of a boolean or a vector type.  */
+
+void
+warn_logical_not_parentheses (location_t location, enum tree_code code,
+			      tree lhs, tree rhs)
+{
+  if (TREE_CODE_CLASS (code) != tcc_comparison
+      || TREE_CODE (TREE_TYPE (lhs)) == BOOLEAN_TYPE
+      || TREE_CODE (TREE_TYPE (rhs)) == BOOLEAN_TYPE
+      || VECTOR_TYPE_P (TREE_TYPE (lhs))
+      || VECTOR_TYPE_P (TREE_TYPE (rhs)))
+    return;
+
+  warning_at (location, OPT_Wlogical_not_parentheses,
+	      "logical not is only applied to the left hand side of "
+	      "comparison");
+}
 
 /* Warn if EXP contains any computations whose results are not used.
    Return true if a warning is printed; false otherwise.  LOCUS is the
diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 0d34004..83d5dee 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -772,6 +772,8 @@  extern void overflow_warning (location_t, tree);
 extern bool warn_if_unused_value (const_tree, location_t);
 extern void warn_logical_operator (location_t, enum tree_code, tree,
 				   enum tree_code, tree, enum tree_code, tree);
+extern void warn_logical_not_parentheses (location_t, enum tree_code, tree,
+					  tree);
 extern void check_main_parameter_types (tree decl);
 extern bool c_determine_visibility (tree);
 extern bool vector_types_compatible_elements_p (tree, tree);
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index c586e65..d51c890 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -490,6 +490,10 @@  Wlogical-op
 C ObjC C++ ObjC++ Var(warn_logical_op) Init(0) Warning 
 Warn when a logical operator is suspiciously always evaluating to true or false
 
+Wlogical-not-parentheses
+C ObjC C++ ObjC++ Var(warn_logical_not_paren) Warning
+Warn when logical not is used on the left hand side operand of a comparison
+
 Wlong-long
 C ObjC C++ ObjC++ Var(warn_long_long) Init(-1) Warning
 Do not warn about using \"long long\" when -pedantic
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index 6ca584b..f3b142f 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3401,6 +3401,11 @@  parser_build_binary_op (location_t location, enum tree_code code,
     warn_logical_operator (location, code, TREE_TYPE (result.value),
 			   code1, arg1.value, code2, arg2.value);
 
+  if (warn_logical_not_paren
+      && code1 == TRUTH_NOT_EXPR
+      && TREE_CODE (arg1.value) == EQ_EXPR)
+    warn_logical_not_parentheses (location, code, arg1.value, arg2.value);
+
   /* Warn about comparisons against string literals, with the exception
      of testing for equality or inequality of a string literal with NULL.  */
   if (code == EQ_EXPR || code == NE_EXPR)
diff --git gcc/cp/parser.c gcc/cp/parser.c
index 2591ae5..1ced05a 100644
--- gcc/cp/parser.c
+++ gcc/cp/parser.c
@@ -7883,6 +7883,8 @@  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 = cp_parser_cast_expression (parser, /*address_p=*/false,
@@ -7985,6 +7987,14 @@  cp_parser_binary_expression (cp_parser* parser, bool cast_p,
       else if (current.tree_type == TRUTH_ORIF_EXPR)
 	c_inhibit_evaluation_warnings -= current.lhs == truthvalue_true_node;
 
+      if (warn_logical_not_paren
+	  && (complain_flags (decltype_p) & tf_warning)
+	  && !processing_template_decl
+	  && TREE_CODE (current.lhs) == EQ_EXPR
+	  && parenthesized_not_lhs_warn)
+	warn_logical_not_parentheses (current.loc, current.tree_type,
+				      TREE_OPERAND (current.lhs, 0), rhs);
+
       overload = NULL;
       /* ??? Currently we pass lhs_type == ERROR_MARK and rhs_type ==
 	 ERROR_MARK for everything that is not a binary expression.
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 9475594..afea361 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -256,7 +256,7 @@  Objective-C and Objective-C++ Dialects}.
 -Winit-self  -Winline -Wmaybe-uninitialized @gol
 -Wno-int-to-pointer-cast -Wno-invalid-offsetof @gol
 -Winvalid-pch -Wlarger-than=@var{len}  -Wunsafe-loop-optimizations @gol
--Wlogical-op -Wlong-long @gol
+-Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol
 -Wmain -Wmaybe-uninitialized -Wmissing-braces  -Wmissing-field-initializers @gol
 -Wmissing-include-dirs @gol
 -Wno-multichar  -Wnonnull  -Wno-overflow -Wopenmp-simd @gol
@@ -4687,6 +4687,24 @@  Warn about suspicious uses of logical operators in expressions.
 This includes using logical operators in contexts where a
 bit-wise operator is likely to be expected.
 
+@item -Wlogical-not-parentheses
+@opindex Wlogical-not-parentheses
+@opindex Wno-logical-not-parentheses
+Warn about logical not used on the left hand side operand of a comparison.
+This option does not warn if the LHS or RHS operand is of a boolean or
+a vector type.  Its purpose is to detect suspicious code like the following:
+@smallexample
+int a;
+...
+if (!a > 1) @{ ... @}
+@end smallexample
+
+It is possible to suppress the warning by wrapping the LHS into
+parentheses:
+@smallexample
+if ((!a) > 1) @{ ... @}
+@end smallexample
+
 @item -Waggregate-return
 @opindex Waggregate-return
 @opindex Wno-aggregate-return
diff --git gcc/testsuite/c-c++-common/pr49706.c gcc/testsuite/c-c++-common/pr49706.c
index e69de29..6a59046 100644
--- gcc/testsuite/c-c++-common/pr49706.c
+++ gcc/testsuite/c-c++-common/pr49706.c
@@ -0,0 +1,95 @@ 
+/* PR c/49706 */
+/* { dg-do compile } */
+/* { dg-options "-Wlogical-not-parentheses" } */
+
+#ifndef __cplusplus
+#define bool _Bool
+#endif
+enum E { A, B };
+bool b;
+extern enum E foo_e (void);
+extern bool foo_b (void);
+extern int foo_i (void);
+
+void
+fn1 (int i1, int i2, bool b1, bool b2)
+{
+  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 = i1 == i2;
+  b = i1 != i2;
+  b = i1 < i2;
+  b = i1 > i2;
+  b = i1 <= i2;
+  b = i1 >= i2;
+
+  /* Parens suppress the warning.  */
+  b = (!i1) == i2;
+  b = (!i1) != i2;
+  b = (!i1) < i2;
+  b = (!i1) > i2;
+  b = (!i1) <= i2;
+  b = (!i1) >= i2;
+
+  /* ...but not these parens.  */
+  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 = !b1 == b2;
+  b = !b1 != b2;
+  b = !b1 < b2;
+  b = !b1 > b2;
+  b = !b1 <= b2;
+  b = !b1 >= b2;
+
+  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;
+  b = !!i1 != i2;
+  b = !!i1 < i2;
+  b = !!i1 > i2;
+  b = !!i1 <= i2;
+  b = !!i1 >= i2;
+  b = !!foo_i () == i1;
+
+  /* Be careful here.  */
+  b = (i1 == 0) != 0;
+  b = (i1 == 0) == 0;
+  b = (i1 != 0) != 0;
+  b = (i1 != 0) == 0;
+}
+
+void
+fn2 (enum E e)
+{
+  b = e == B;
+  b = e == foo_e ();
+  b = foo_e () == A;
+  b = foo_e () == foo_e ();
+
+  b = !e == A; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !e == foo_e (); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !foo_e () == A; /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+  b = !foo_e () == foo_e (); /* { dg-warning "logical not is only applied to the left hand side of comparison" } */
+
+  b = !(e == A);
+  b = !(e == foo_e ());
+  b = !(foo_e () == A);
+  b = !(foo_e () == foo_e ());
+
+  b = (!e) == A;
+  b = (!e) == foo_e ();
+  b = (!foo_e ()) == A;
+  b = (!foo_e ()) == foo_e ();
+}