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 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 ();
+}
Marek
