Message ID | 20151118161624.GD21807@redhat.com |
---|---|
State | New |
Headers | show |
On 11/18/2015 05:16 PM, Marek Polacek wrote: > Actually, no, I think we should do this instead. > > +++ gcc/c-family/c-common.c > @@ -1924,7 +1924,7 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs) > > /* We do not warn for constants because they are typical of macro > expansions that test for features, sizeof, and similar. */ > - if (CONSTANT_CLASS_P (fold (lhs)) || CONSTANT_CLASS_P (fold (rhs))) > + if (CONSTANT_CLASS_P (lhs) || CONSTANT_CLASS_P (rhs)) > return; > > /* Don't warn for e.g. > diff --git gcc/cp/call.c gcc/cp/call.c > index 8cdda62..77c2936 100644 > --- gcc/cp/call.c > +++ gcc/cp/call.c > @@ -5741,7 +5741,7 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1, > maybe_warn_bool_compare (loc, code, fold (arg1), > fold (arg2)); > if (complain & tf_warning && warn_tautological_compare) > - warn_tautological_cmp (loc, code, arg1, arg2); > + warn_tautological_cmp (loc, code, fold (arg1), fold (arg2)); > /* Fall through. */ > case PLUS_EXPR: > case MINUS_EXPR: That seems to change the behaviour of the code though. Most of the code in warn_tautological_cmp only looks at the unfolded form. Are you sure this is safe, for example wrt. the from_macro_expansion_at tests or the CONVERT_EXPR_P ones? Bernd
On Wed, Nov 18, 2015 at 05:24:34PM +0100, Bernd Schmidt wrote: > That seems to change the behaviour of the code though. Most of the code in > warn_tautological_cmp only looks at the unfolded form. Are you sure this is > safe, for example wrt. the from_macro_expansion_at tests or the > CONVERT_EXPR_P ones? Of course the second version is wrong wrt CONVERT_EXPR_P, because it would regress g++.dg/warn/Wtautological-compare.C -- with the fold() in the C++ FE we'd get rid of the CONVERT_EXPR_P completely and we'd warn. Sorry :(. So I guess scratch the second version and let's get back to the first one. Jason's r230471 was a correct thing to do. Marek
diff --git gcc/c-family/c-common.c gcc/c-family/c-common.c index f50ca48..06d857c 100644 --- gcc/c-family/c-common.c +++ gcc/c-family/c-common.c @@ -1924,7 +1924,7 @@ warn_tautological_cmp (location_t loc, enum tree_code code, tree lhs, tree rhs) /* We do not warn for constants because they are typical of macro expansions that test for features, sizeof, and similar. */ - if (CONSTANT_CLASS_P (fold (lhs)) || CONSTANT_CLASS_P (fold (rhs))) + if (CONSTANT_CLASS_P (lhs) || CONSTANT_CLASS_P (rhs)) return; /* Don't warn for e.g. diff --git gcc/cp/call.c gcc/cp/call.c index 8cdda62..77c2936 100644 --- gcc/cp/call.c +++ gcc/cp/call.c @@ -5741,7 +5741,7 @@ build_new_op_1 (location_t loc, enum tree_code code, int flags, tree arg1, maybe_warn_bool_compare (loc, code, fold (arg1), fold (arg2)); if (complain & tf_warning && warn_tautological_compare) - warn_tautological_cmp (loc, code, arg1, arg2); + warn_tautological_cmp (loc, code, fold (arg1), fold (arg2)); /* Fall through. */ case PLUS_EXPR: case MINUS_EXPR: diff --git gcc/testsuite/gcc.dg/pr68412.c gcc/testsuite/gcc.dg/pr68412.c index e69de29..825eb63 100644 --- gcc/testsuite/gcc.dg/pr68412.c +++ gcc/testsuite/gcc.dg/pr68412.c @@ -0,0 +1,41 @@ +/* PR c/68412 */ +/* { dg-do compile } */ +/* { dg-options "-Wall -Wextra" } */ + +#define M (sizeof (int) * __CHAR_BIT__) + +int +fn1 (int i) +{ + return i == (-1 << 8); /* { dg-warning "left shift of negative value" } */ +} + +int +fn2 (int i) +{ + return i == (1 << M); /* { dg-warning "left shift count" } */ +} + +int +fn3 (int i) +{ + return i == 10 << (M - 1); /* { dg-warning "requires" } */ +} + +int +fn4 (int i) +{ + return i == 1 << -1; /* { dg-warning "left shift count" } */ +} + +int +fn5 (int i) +{ + return i == 1 >> M; /* { dg-warning "right shift count" } */ +} + +int +fn6 (int i) +{ + return i == 1 >> -1; /* { dg-warning "right shift count" } */ +}