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

login
register
mail settings
Submitter Marek Polacek
Date June 2, 2014, 3:17 p.m.
Message ID <20140602151732.GA11694@redhat.com>
Download mbox | patch
Permalink /patch/354964/
State New
Headers show

Comments

Marek Polacek - June 2, 2014, 3:17 p.m.
On Mon, Jun 02, 2014 at 10:08:24AM -0400, Jason Merrill wrote:
> 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.
 
Actually it warns about all tcc_comparison cases; the reason for EQ_EXPR
checks above was that we convert the LHS from e.g. "!x" into "x == 0"
(at least in the C FE this is done in c_objc_common_truthvalue_conversion
+ invert_truthvalue), so I checked that.  But it's useless, so I dropped
those checks.

> >+	  && (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.
 
Good, I dropped that check too.

> >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.

Thanks.

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, 4:01 p.m.
On 06/02/2014 11:17 AM, Marek Polacek wrote:
> +	  && !processing_template_decl

Also, why not warn when parsing a template?  We don't need to know the 
type to recognize the problematic syntax.

Jason
Joseph S. Myers - June 4, 2014, 10:47 p.m.
On Mon, 2 Jun 2014, Marek Polacek wrote:

> +@smallexample
> +int a;
> +...
> +if (!a > 1) @{ ... @}

@dots{}, since this is an ellipsis not the literal ... token.
Marek Polacek - June 5, 2014, 5:58 a.m.
On Wed, Jun 04, 2014 at 10:47:34PM +0000, Joseph S. Myers wrote:
> On Mon, 2 Jun 2014, Marek Polacek wrote:
> 
> > +@smallexample
> > +int a;
> > +...
> > +if (!a > 1) @{ ... @}
> 
> @dots{}, since this is an ellipsis not the literal ... token.

Fixed (and on the other spot in -Wswitch-bool paragraph too), thanks.

	Marek

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..e98224e 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -3401,6 +3401,10 @@  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)
+    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..2e595f3 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,12 @@  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
+	  && !processing_template_decl
+	  && 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..9ad6444 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; /* { 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" } */
+
+  /* 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 ();
+}