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

2014-06-23 Thread Marek Polacek 
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  pola...@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.

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;
+}

Marek

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

2014-06-23 Thread Joseph S. Myers 
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  pola...@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.

-- 
Joseph S. Myers
jos...@codesourcery.com

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

2014-06-22 Thread Gerald Pfeifer 
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

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

2014-06-04 Thread Marek Polacek 
On Mon, Jun 02, 2014 at 01:50:53PM -0400, Jason Merrill wrote:
 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 0x7013d5e8 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.

So is the following any better?  I added handling of NULL_TYPES (we
warn in that case), and a little bit of testing.  I also verified
that we don't warn if the LHS is wrapped in parens.
But whether this is good enough, or whether I need
type_dependent_expression_p, I don't know.

Tested x86_64.

2014-06-04  Marek Polacek  pola...@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.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 07a1798..fd70ead 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1722,6 +1722,28 @@ 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_TYPE (lhs) == NULL_TREE
+  || TREE_TYPE (rhs) == NULL_TREE)
+;
+  else 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 5d36a80..76e67d7 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 a98ce07..e0d3fde 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

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

2014-06-04 Thread Jakub Jelinek 
On Wed, Jun 04, 2014 at 10:56:43PM +0200, Marek Polacek wrote:

 +/* 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_TYPE (lhs) == NULL_TREE
 +  || TREE_TYPE (rhs) == NULL_TREE)
 +;
 +  else if (TREE_CODE_CLASS (code) != tcc_comparison

Shouldn't this test be done first?  I mean, if type is NULL on lhs or rhs,
you'll warn even when code is not 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);
 +}
  

Jakub

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

2014-06-04 Thread Marek Polacek 
On Wed, Jun 04, 2014 at 11:02:10PM +0200, Jakub Jelinek wrote:
 On Wed, Jun 04, 2014 at 10:56:43PM +0200, Marek Polacek wrote:
 
  +/* 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_TYPE (lhs) == NULL_TREE
  +  || TREE_TYPE (rhs) == NULL_TREE)
  +;
  +  else if (TREE_CODE_CLASS (code) != tcc_comparison
 
 Shouldn't this test be done first?  I mean, if type is NULL on lhs or rhs,
 you'll warn even when code is not comparison...

Obviously it's too late for me :^).  Hopefully fixed...

2014-06-04  Marek Polacek  pola...@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.

diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index 07a1798..fb6c612 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -1722,6 +1722,29 @@ 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)
+return;
+  if (TREE_TYPE (lhs) == NULL_TREE
+  || TREE_TYPE (rhs) == NULL_TREE)
+;
+  else if (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 5d36a80..76e67d7 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 a98ce07..e0d3fde 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;

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

2014-06-04 Thread Jason Merrill 

OK.

Jason

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

2014-06-04 Thread Joseph S. Myers 
On Mon, 2 Jun 2014, Marek Polacek wrote:

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

@dots{}, since this is an ellipsis not the literal ... token.

-- 
Joseph S. Myers
jos...@codesourcery.com

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

2014-06-04 Thread Marek Polacek 
On Wed, Jun 04, 2014 at 10:47:34PM +, 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

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

2014-06-02 Thread Marek Polacek 
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  pola...@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.

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

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

2014-06-02 Thread Jason Merrill 

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

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

2014-06-02 Thread Marek Polacek 
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  pola...@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.

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

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

2014-06-02 Thread Jason Merrill 

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

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

2014-06-02 Thread Marek Polacek 
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  pola...@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.

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)
+

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

2014-06-02 Thread Jason Merrill 

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

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

2014-06-02 Thread Jakub Jelinek 
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

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

2014-06-02 Thread Marek Polacek 
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 0x7013d5e8 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

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

2014-06-02 Thread Jakub Jelinek 
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

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

2014-06-02 Thread Jason Merrill 

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 0x7013d5e8 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