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 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.
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 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 1c2e079..789d702 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
@@ -4704,6 +4704,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..615f3e4 100644
--- gcc/testsuite/c-c++-common/pr49706.c
+++ gcc/testsuite/c-c++-common/pr49706.c
@@ -0,0 +1,102 @@
+/* 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);
+
+#ifdef __cplusplus
+template <class T, class U> bool f1(T t, U u) { return (!t == u); } /* {
dg-warning "logical not is only applied to the left hand side of comparison" ""
{ target c++ } 15 } */
+template <class T, class U> bool f2(T t, U u) { return ((!t) == u); }
+template <class T, class U> bool f3(T t, U u) { return (!g(t) == u); } /* {
dg-warning "logical not is only applied to the left hand side of comparison" ""
{ target c++ } 17 } */
+template <class T, class U> bool f4(T t, U u) { return ((!g(t)) == u); }
+#endif
+
+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 ();
+}
Marek
