diff mbox

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

Message ID 20140602162300.GD11694@redhat.com
State New
Headers show

Commit Message

Marek Polacek June 2, 2014, 4:23 p.m. UTC 
On Mon, Jun 02, 2014 at 12:01:22PM -0400, Jason Merrill wrote:
> 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.

I have no special reason for that check, so dropped it too in this
patch.

Ran dg.exp with this warning enabled in -Wall; no regressions.

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

Comments

Jason Merrill June 2, 2014, 4:31 p.m. UTC | #1 
On 06/02/2014 12:23 PM, Marek Polacek wrote:
> I have no special reason for that check, so dropped it too in this
> patch.

Thanks. I expect that warn_logical_not_parentheses will crash if one of 
the operands is type-dependent such that TREE_TYPE is NULL_TREE, so 
you'll want to handle that case, and test it in the testcase, i.e. 
something like

#ifdef __cplusplus
template <class T, class U> bool f(T t, U u) { return (!t == u); }
#endif

I think !t should have null TREE_TYPE in this case.

Jason
Jakub Jelinek June 2, 2014, 4:36 p.m. UTC | #2 
On Mon, Jun 02, 2014 at 12:31:32PM -0400, Jason Merrill wrote:
> On 06/02/2014 12:23 PM, Marek Polacek wrote:
> >I have no special reason for that check, so dropped it too in this
> >patch.
> 
> Thanks. I expect that warn_logical_not_parentheses will crash if one
> of the operands is type-dependent such that TREE_TYPE is NULL_TREE,
> so you'll want to handle that case, and test it in the testcase,
> i.e. something like
> 
> #ifdef __cplusplus
> template <class T, class U> bool f(T t, U u) { return (!t == u); }
> #endif
> 
> I think !t should have null TREE_TYPE in this case.

Do we actually want to warn in that case?  As the patch doesn't warn
if the type is bool or vector, if somebody instantiates the above with
say T=bool, U=bool, then we'd warn on something that otherwise will not be
warned about when not in template.
But to warn about this during instantiation, we'd need to somehow tell
tsubst* whether it was !t == u or (!t) == u ...

	Jakub
Marek Polacek June 2, 2014, 5:04 p.m. UTC | #3 
On Mon, Jun 02, 2014 at 06:36:06PM +0200, Jakub Jelinek wrote:
> On Mon, Jun 02, 2014 at 12:31:32PM -0400, Jason Merrill wrote:
> > On 06/02/2014 12:23 PM, Marek Polacek wrote:
> > >I have no special reason for that check, so dropped it too in this
> > >patch.
> > 
> > Thanks. I expect that warn_logical_not_parentheses will crash if one
> > of the operands is type-dependent such that TREE_TYPE is NULL_TREE,
> > so you'll want to handle that case, and test it in the testcase,
> > i.e. something like
> > 
> > #ifdef __cplusplus
> > template <class T, class U> bool f(T t, U u) { return (!t == u); }
> > #endif
> > 
> > I think !t should have null TREE_TYPE in this case.
 
Hmm, I see no crash; the types seem to be
template_type_parm 0x7ffff013d5e8 T type_0 type_6 VOID ...

> Do we actually want to warn in that case?  As the patch doesn't warn
> if the type is bool or vector, if somebody instantiates the above with
> say T=bool, U=bool, then we'd warn on something that otherwise will not be
> warned about when not in template.
> But to warn about this during instantiation, we'd need to somehow tell
> tsubst* whether it was !t == u or (!t) == u ...

Ah - indeed.  So I suggest to stay with !processing_template_decl ;).

	Marek
Jakub Jelinek June 2, 2014, 5:14 p.m. UTC | #4 
On Mon, Jun 02, 2014 at 07:04:58PM +0200, Marek Polacek wrote:
> > Do we actually want to warn in that case?  As the patch doesn't warn
> > if the type is bool or vector, if somebody instantiates the above with
> > say T=bool, U=bool, then we'd warn on something that otherwise will not be
> > warned about when not in template.
> > But to warn about this during instantiation, we'd need to somehow tell
> > tsubst* whether it was !t == u or (!t) == u ...
> 
> Ah - indeed.  So I suggest to stay with !processing_template_decl ;).

Well, if the vars are known not to be boolean, even in template, you can
warn about this right away (i.e. if they aren't
type_dependent_expression_p).

	Jakub
Jason Merrill June 2, 2014, 5:50 p.m. UTC | #5 
On 06/02/2014 01:04 PM, Marek Polacek wrote:
>>> #ifdef __cplusplus
>>> template <class T, class U> bool f(T t, U u) { return (!t == u); }
>>> #endif
>>>
>>> I think !t should have null TREE_TYPE in this case.
>
> Hmm, I see no crash; the types seem to be
> template_type_parm 0x7ffff013d5e8 T type_0 type_6 VOID ...

Right, because you're pulling out the operand of the not.  OK, then you 
need something a little more complicated like !g(t).

>> Do we actually want to warn in that case?  As the patch doesn't warn
>> if the type is bool or vector, if somebody instantiates the above with
>> say T=bool, U=bool, then we'd warn on something that otherwise will not be
>> warned about when not in template.

I think we still want to warn.  A template that is only correct for one 
possible template argument shouldn't be a template.

Jason
diff mbox

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..60e6cda 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,11 @@  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
+	  && 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 ();
+}