On Mon, Aug 18, 2014 at 10:23:49PM +0200, Marek Polacek wrote:
> On Mon, Aug 18, 2014 at 04:10:49PM -0400, Jason Merrill wrote:
> > On 08/18/2014 04:04 PM, Marek Polacek wrote:
> > >Unfortunately, this warning cannot be enabled by -Wall yet, because
> > >of a few blunders we have in the codebase. But we really should iron out
> > >that soon.
> >
> > Can you take care of that as well?
>
> Yea - I actually meant to do that, but then I saw this weirdo code in
> optabs.c and got cold feet.
>
> > The patch looks good to me, but if it should be part of -Wall I want there
> > to be a plan beyond "someone should fix it". :)
>
> Okay, I'll ping this patch when it's ready for -Wall.
I've fixed a few issues that -Wbool-compare found, so it can be
enabled by -Wall now. This patch is pretty much the same as the
last version except that
a) the warning is enabled by -Wall,
b) maybe_warn_bool_compare was slightly revamped as per Manuel's
suggestion (the semantics is the same).
Bootstrapped/regtested on x86_64-linux, ok for trunk?
2014-08-19 Marek Polacek <pola...@redhat.com>
PR c++/62153
* doc/invoke.texi: Document -Wbool-compare.
c-family/
* c-common.c (maybe_warn_bool_compare): New function.
* c-common.h (maybe_warn_bool_compare): Declare.
* c.opt (Wbool-compare): New option.
c/
* c-typeck.c (build_binary_op): If either operand of a comparison
is a boolean expression, call maybe_warn_bool_compare.
cp/
* call.c (build_new_op_1): Remember the type of arguments for
a comparison. If either operand of a comparison is a boolean
expression, call maybe_warn_bool_compare.
testsuite/
* c-c++-common/Wbool-compare-1.c: New test.
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c
index acc9a20..c69ab3e 100644
--- gcc/c-family/c-common.c
+++ gcc/c-family/c-common.c
@@ -11612,6 +11612,39 @@ maybe_warn_unused_local_typedefs (void)
vec_free (l->local_typedefs);
}
+/* Warn about boolean expression compared with an integer value different
+ from true/false. Warns also e.g. about "(i1 == i2) == 2".
+ LOC is the location of the comparison, CODE is its code, OP0 and OP1
+ are the operands of the comparison. The caller must ensure that
+ either operand is a boolean expression. */
+
+void
+maybe_warn_bool_compare (location_t loc, enum tree_code code, tree op0,
+ tree op1)
+{
+ if (TREE_CODE_CLASS (code) != tcc_comparison)
+ return;
+
+ tree cst = TREE_CODE (op0) == INTEGER_CST
+ ? op0 : TREE_CODE (op1) == INTEGER_CST ? op1 : NULL_TREE;
+ if (!cst)
+ return;
+
+ if (!integer_zerop (cst) && !integer_onep (cst))
+ {
+ int sign = TREE_CODE (op0) == INTEGER_CST
+ ? tree_int_cst_sgn (cst) : -tree_int_cst_sgn (cst);
+ if (code == EQ_EXPR
+ || ((code == GT_EXPR || code == GE_EXPR) && sign < 0)
+ || ((code == LT_EXPR || code == LE_EXPR) && sign > 0))
+ warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
+ "with boolean expression is always false", cst);
+ else
+ warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
+ "with boolean expression is always true", cst);
+ }
+}
+
/* The C and C++ parsers both use vectors to hold function arguments.
For efficiency, we keep a cache of unused vectors. This is the
cache. */
diff --git gcc/c-family/c-common.h gcc/c-family/c-common.h
index 26aaee2..995bc8c 100644
--- gcc/c-family/c-common.h
+++ gcc/c-family/c-common.h
@@ -1015,6 +1015,7 @@ extern void record_types_used_by_current_var_decl (tree);
extern void record_locally_defined_typedef (tree);
extern void maybe_record_typedef_use (tree);
extern void maybe_warn_unused_local_typedefs (void);
+extern void maybe_warn_bool_compare (location_t, enum tree_code, tree, tree);
extern vec<tree, va_gc> *make_tree_vector (void);
extern void release_tree_vector (vec<tree, va_gc> *);
extern vec<tree, va_gc> *make_tree_vector_single (tree);
diff --git gcc/c-family/c.opt gcc/c-family/c.opt
index 4848399..f97a11a 100644
--- gcc/c-family/c.opt
+++ gcc/c-family/c.opt
@@ -287,6 +287,10 @@ Wbad-function-cast
C ObjC Var(warn_bad_function_cast) Warning
Warn about casting functions to incompatible types
+Wbool-compare
+C ObjC C++ ObjC++ Var(warn_bool_compare) Warning LangEnabledBy(C ObjC C++
ObjC++,Wall)
+Warn about boolean expression compared with an integer value different from
true/false
+
Wbuiltin-macro-redefined
C ObjC C++ ObjC++ Warning
Warn when a built-in preprocessor macro is undefined or redefined
diff --git gcc/c/c-typeck.c gcc/c/c-typeck.c
index b1eac34..91c900d 100644
--- gcc/c/c-typeck.c
+++ gcc/c/c-typeck.c
@@ -10676,6 +10676,11 @@ build_binary_op (location_t location, enum tree_code
code,
result_type = type1;
pedwarn (location, 0, "comparison between pointer and integer");
}
+ if ((TREE_CODE (TREE_TYPE (orig_op0)) == BOOLEAN_TYPE
+ || truth_value_p (TREE_CODE (orig_op0)))
+ ^ (TREE_CODE (TREE_TYPE (orig_op1)) == BOOLEAN_TYPE
+ || truth_value_p (TREE_CODE (orig_op1))))
+ maybe_warn_bool_compare (location, code, orig_op0, orig_op1);
break;
case LE_EXPR:
@@ -10780,6 +10785,11 @@ build_binary_op (location_t location, enum tree_code
code,
result_type = type1;
pedwarn (location, 0, "comparison between pointer and integer");
}
+ if ((TREE_CODE (TREE_TYPE (orig_op0)) == BOOLEAN_TYPE
+ || truth_value_p (TREE_CODE (orig_op0)))
+ ^ (TREE_CODE (TREE_TYPE (orig_op1)) == BOOLEAN_TYPE
+ || truth_value_p (TREE_CODE (orig_op1))))
+ maybe_warn_bool_compare (location, code, orig_op0, orig_op1);
break;
default:
diff --git gcc/cp/call.c gcc/cp/call.c
index 7044e14..161235b 100644
--- gcc/cp/call.c
+++ gcc/cp/call.c
@@ -5318,7 +5318,17 @@ build_new_op_1 (location_t loc, enum tree_code code, int
flags, tree arg1,
/* These are saved for the sake of warn_logical_operator. */
code_orig_arg1 = TREE_CODE (arg1);
code_orig_arg2 = TREE_CODE (arg2);
-
+ break;
+ case GT_EXPR:
+ case LT_EXPR:
+ case GE_EXPR:
+ case LE_EXPR:
+ case EQ_EXPR:
+ case NE_EXPR:
+ /* These are saved for the sake of maybe_warn_bool_compare. */
+ code_orig_arg1 = TREE_CODE (TREE_TYPE (arg1));
+ code_orig_arg2 = TREE_CODE (TREE_TYPE (arg2));
+ break;
default:
break;
}
@@ -5625,16 +5635,20 @@ build_new_op_1 (location_t loc, enum tree_code code,
int flags, tree arg1,
warn_logical_operator (loc, code, boolean_type_node,
code_orig_arg1, arg1, code_orig_arg2, arg2);
/* Fall through. */
- case PLUS_EXPR:
- case MINUS_EXPR:
- case MULT_EXPR:
- case TRUNC_DIV_EXPR:
case GT_EXPR:
case LT_EXPR:
case GE_EXPR:
case LE_EXPR:
case EQ_EXPR:
case NE_EXPR:
+ if ((code_orig_arg1 == BOOLEAN_TYPE)
+ ^ (code_orig_arg2 == BOOLEAN_TYPE))
+ maybe_warn_bool_compare (loc, code, arg1, arg2);
+ /* Fall through. */
+ case PLUS_EXPR:
+ case MINUS_EXPR:
+ case MULT_EXPR:
+ case TRUNC_DIV_EXPR:
case MAX_EXPR:
case MIN_EXPR:
case LSHIFT_EXPR:
diff --git gcc/doc/invoke.texi gcc/doc/invoke.texi
index 8ea3689..b8e4294 100644
--- gcc/doc/invoke.texi
+++ gcc/doc/invoke.texi
@@ -240,6 +240,7 @@ Objective-C and Objective-C++ Dialects}.
-pedantic-errors @gol
-w -Wextra -Wall -Waddress -Waggregate-return @gol
-Waggressive-loop-optimizations -Warray-bounds @gol
+-Wbool-compare @gol
-Wno-attributes -Wno-builtin-macro-redefined @gol
-Wc90-c99-compat -Wc99-c11-compat @gol
-Wc++-compat -Wc++11-compat -Wcast-align -Wcast-qual @gol
@@ -4221,6 +4222,19 @@ This option is only active when @option{-ftree-vrp} is
active
(default for @option{-O2} and above). It warns about subscripts to arrays
that are always out of bounds. This warning is enabled by @option{-Wall}.
+@item -Wbool-compare
+@opindex Wno-bool-compare
+@opindex Wbool-compare
+Warn about boolean expression compared with an integer value different from
+@code{true}/@code{false}. For instance, the following comparison is
+always false:
+@smallexample
+int n = 5;
+@dots{}
+if ((n > 1) == 2) @{ @dots{} @}
+@end smallexample
+This warning is enabled by @option{-Wall}.
+
@item -Wno-discarded-qualifiers @r{(C and Objective-C only)}
@opindex Wno-discarded-qualifiers
@opindex Wdiscarded-qualifiers
diff --git gcc/testsuite/c-c++-common/Wbool-compare-1.c
gcc/testsuite/c-c++-common/Wbool-compare-1.c
index e69de29..5b03e06 100644
--- gcc/testsuite/c-c++-common/Wbool-compare-1.c
+++ gcc/testsuite/c-c++-common/Wbool-compare-1.c
@@ -0,0 +1,128 @@
+/* PR c++/62153 */
+/* { dg-do compile } */
+/* { dg-options "-Wall" } */
+
+#ifndef __cplusplus
+# define bool _Bool
+# define true 1
+# define false 0
+#endif
+
+extern bool foo (void);
+bool r;
+
+enum { E = 4 };
+
+void
+fn1 (bool b)
+{
+ r = b == 2; /* { dg-warning "with boolean expression is always false" } */
+ r = b != 2; /* { dg-warning "with boolean expression is always true" } */
+ r = b < 2; /* { dg-warning "with boolean expression is always true" } */
+ r = b > 2; /* { dg-warning "with boolean expression is always false" } */
+ r = b <= 2; /* { dg-warning "with boolean expression is always true" } */
+ r = b >= 2; /* { dg-warning "with boolean expression is always false" } */
+
+ r = b == -1; /* { dg-warning "with boolean expression is always false" } */
+ r = b != -1; /* { dg-warning "with boolean expression is always true" } */
+ r = b < -1; /* { dg-warning "with boolean expression is always false" } */
+ r = b > -1; /* { dg-warning "with boolean expression is always true" } */
+ r = b <= -1; /* { dg-warning "with boolean expression is always false" } */
+ r = b >= -1; /* { dg-warning "with boolean expression is always true" } */
+
+ r = foo () == 2; /* { dg-warning "with boolean expression is always false" }
*/
+ r = foo () != 2; /* { dg-warning "with boolean expression is always true" }
*/
+ r = foo () < 2; /* { dg-warning "with boolean expression is always true" } */
+ r = foo () > 2; /* { dg-warning "with boolean expression is always false" }
*/
+ r = foo () <= 2; /* { dg-warning "with boolean expression is always true" }
*/
+ r = foo () >= 2; /* { dg-warning "with boolean expression is always false" }
*/
+
+ r = b == E; /* { dg-warning "with boolean expression is always false" } */
+ r = b != E; /* { dg-warning "with boolean expression is always true" } */
+ r = b < E; /* { dg-warning "with boolean expression is always true" } */
+ r = b > E; /* { dg-warning "with boolean expression is always false" } */
+ r = b <= E; /* { dg-warning "with boolean expression is always true" } */
+ r = b >= E; /* { dg-warning "with boolean expression is always false" } */
+
+ /* Swap LHS and RHS. */
+ r = 2 == b; /* { dg-warning "with boolean expression is always false" } */
+ r = 2 != b; /* { dg-warning "with boolean expression is always true" } */
+ r = 2 < b; /* { dg-warning "with boolean expression is always false" } */
+ r = 2 > b; /* { dg-warning "with boolean expression is always true" } */
+ r = 2 <= b; /* { dg-warning "with boolean expression is always false" } */
+ r = 2 >= b; /* { dg-warning "with boolean expression is always true" } */
+
+ r = -1 == b; /* { dg-warning "with boolean expression is always false" } */
+ r = -1 != b; /* { dg-warning "with boolean expression is always true" } */
+ r = -1 < b; /* { dg-warning "with boolean expression is always true" } */
+ r = -1 > b; /* { dg-warning "with boolean expression is always false" } */
+ r = -1 <= b; /* { dg-warning "with boolean expression is always true" } */
+ r = -1 >= b; /* { dg-warning "with boolean expression is always false" } */
+
+ r = E == b; /* { dg-warning "with boolean expression is always false" } */
+ r = E != b; /* { dg-warning "with boolean expression is always true" } */
+ r = E < b; /* { dg-warning "with boolean expression is always false" } */
+ r = E > b; /* { dg-warning "with boolean expression is always true" } */
+ r = E <= b; /* { dg-warning "with boolean expression is always false" } */
+ r = E >= b; /* { dg-warning "with boolean expression is always true" } */
+
+ /* These are of course fine. */
+ r = b == false;
+ r = b != false;
+ r = b == true;
+ r = b != true;
+
+ /* Some of these don't make much sense, but we don't warn. */
+ r = b < false;
+ r = b >= false;
+ r = b <= false;
+ r = b > false;
+ r = b < true;
+ r = b >= true;
+ r = b <= true;
+ r = b > true;
+}
+
+void
+fn2 (int i1, int i2)
+{
+ r = (i1 == i2) == 2; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) != 2; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) < 2; /* { dg-warning "with boolean expression is always true"
} */
+ r = (i1 == i2) > 2; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) <= 2; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) >= 2; /* { dg-warning "with boolean expression is always
false" } */
+
+ r = (i1 == i2) == -1; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) != -1; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) < -1; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) > -1; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) <= -1; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) >= -1; /* { dg-warning "with boolean expression is always
true" } */
+
+ r = (i1 == i2) == E; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) != E; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) < E; /* { dg-warning "with boolean expression is always true"
} */
+ r = (i1 == i2) > E; /* { dg-warning "with boolean expression is always
false" } */
+ r = (i1 == i2) <= E; /* { dg-warning "with boolean expression is always
true" } */
+ r = (i1 == i2) >= E; /* { dg-warning "with boolean expression is always
false" } */
+}
+
+void
+fn3 (int n, bool b)
+{
+ /* Don't warn here. */
+ r = b == n;
+ r = b != n;
+ r = b < n;
+ r = b > n;
+ r = b <= n;
+ r = b >= n;
+
+ r = n == E;
+ r = n != E;
+ r = n < E;
+ r = n > E;
+ r = n <= E;
+ r = n >= E;
+}
Marek
