| Message ID | AM4PR0701MB2162918E947B0A81ED74C940E4F10@AM4PR0701MB2162.eurprd07.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > The other false positive was in dwarf2out, where we have this: > > ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer > constants in boolean context [-Werror=int-in-bool-context] > if (s->refcount > == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2)) > ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ > > and: > #define DEBUG_STR_SECTION_FLAGS \ > (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings \ > ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1 \ > : SECTION_DEBUG) > > which got folded in C++ to > if (s->refcount == > ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2)) Hmm, I'd think this warning should be looking at unfolded trees. > Additional to what you suggested, I relaxed the condition even more, > because I think it is also suspicious, if one of the branches is known > to be outside [0:1] and the other is unknown. > > That caused another warning in the fortran FE, which was caused by > wrong placement of braces in gfc_simplify_repeat. > > We have: > #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0) > > Therefore the condition must be .. && mpz_sgn(X) != 0) > > Previously that did not match, because -1 is outside [0:1] > but (Z)->size > 0 is unknown. But (Z)->size > 0 is known to be boolean; that seems like it should suppress this warning. > Furthermore the C++ test case df-warn-signedunsigned1.C also > triggered the warning, because here we have: > > #define MAK (fl < 0 ? 1 : (fl ? -1 : 0)) > > And thus "if (MAK)" should be written as if (MAK != 0) This also seems like a false positive to me. It's very common for code to rely on the implicit !=0 in boolean conversion; it seems to me that the generalization to warn about expressions with 0 arms significantly increases the frequency of false positives, making the flag less useful. The original form of the warning seems like a -Wall candidate to me, but the current form doesn't. Jason
On 09/14/16 18:37, Jason Merrill wrote: > On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger > <bernd.edlinger@hotmail.de> wrote: >> The other false positive was in dwarf2out, where we have this: >> >> ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer >> constants in boolean context [-Werror=int-in-bool-context] >> if (s->refcount >> == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2)) >> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ >> >> and: >> #define DEBUG_STR_SECTION_FLAGS \ >> (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings \ >> ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1 \ >> : SECTION_DEBUG) >> >> which got folded in C++ to >> if (s->refcount == >> ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2)) > > Hmm, I'd think this warning should be looking at unfolded trees. > Yes. The truthvalue_conversion is happening in cp_convert_and_check at cp_convert, afterwards the cp_fully_fold happens and does this transformation that I did not notice before, but just because I did not have the right test case. Then if the folding did change something the truthvalue_conversion happens again, and this time the tree is folded potentially in a surprising way. The new version of the patch disables the warning in the second truthvalue_conversion and as a consequence has to use fold_for_warn on the now unfolded parameters. This looks like an improvement that I would keep, because I would certainly like to warn on x ? 1 : 1+1, which the previous version did, but just by accident. >> Additional to what you suggested, I relaxed the condition even more, >> because I think it is also suspicious, if one of the branches is known >> to be outside [0:1] and the other is unknown. >> >> That caused another warning in the fortran FE, which was caused by >> wrong placement of braces in gfc_simplify_repeat. >> >> We have: >> #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0) >> >> Therefore the condition must be .. && mpz_sgn(X) != 0) >> >> Previously that did not match, because -1 is outside [0:1] >> but (Z)->size > 0 is unknown. > > But (Z)->size > 0 is known to be boolean; that seems like it should > suppress this warning. > Hmm, maybe it got a bit too pedantic, but in some cases this warning is pointing out something that is at least questionable and in some cases real bugs: For instance PR 77574, where in gcc/predict.c we had this: /* If edge is already improbably or cold, just return. */ if (e->probability <= impossible ? PROB_VERY_UNLIKELY : 0 && (!impossible || !e->count)) return; thus if (x <= y ? 4999 : 0 && (...)) >> Furthermore the C++ test case df-warn-signedunsigned1.C also >> triggered the warning, because here we have: >> >> #define MAK (fl < 0 ? 1 : (fl ? -1 : 0)) >> >> And thus "if (MAK)" should be written as if (MAK != 0) > > This also seems like a false positive to me. > > It's very common for code to rely on the implicit !=0 in boolean > conversion; it seems to me that the generalization to warn about > expressions with 0 arms significantly increases the frequency of false > positives, making the flag less useful. The original form of the > warning seems like a -Wall candidate to me, but the current form > doesn't. > Yes. The reasoning I initially had was that it is completely pointless to have something of the form "if (x ? 1 : 2)" or "if (x ? 0 : 0)" because the result does not even depend on x in this case. But something like "if (x ? 4999 : 0)" looks bogus but does at least not ignore x. If the false-positives are becoming too much of a problem here, then I should of course revert to the previous heuristic again. Thanks Bernd.
On Wed, Sep 14, 2016 at 04:10:46PM +0000, Bernd Edlinger wrote: > > fortran: > 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> > > PR c++/77434 > * simplify.c (gfc_simplify_repeat): Fix a warning. > > Index: gcc/fortran/simplify.c > =================================================================== > --- gcc/fortran/simplify.c (revision 240135) > +++ gcc/fortran/simplify.c (working copy) > @@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n) > > if (len || > (e->ts.u.cl->length && > - mpz_sgn (e->ts.u.cl->length->value.integer)) != 0) > + mpz_sgn (e->ts.u.cl->length->value.integer) != 0)) > { > const char *res = gfc_extract_int (n, &ncop); > gcc_assert (res == NULL); This part should be committed regardless of the outcome of a review of the complete patch. The closing ')' is clearly missed placed.
On 09/14/16 19:40, Steve Kargl wrote: > On Wed, Sep 14, 2016 at 04:10:46PM +0000, Bernd Edlinger wrote: >> >> fortran: >> 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> >> >> PR c++/77434 >> * simplify.c (gfc_simplify_repeat): Fix a warning. >> >> Index: gcc/fortran/simplify.c >> =================================================================== >> --- gcc/fortran/simplify.c (revision 240135) >> +++ gcc/fortran/simplify.c (working copy) >> @@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n) >> >> if (len || >> (e->ts.u.cl->length && >> - mpz_sgn (e->ts.u.cl->length->value.integer)) != 0) >> + mpz_sgn (e->ts.u.cl->length->value.integer) != 0)) >> { >> const char *res = gfc_extract_int (n, &ncop); >> gcc_assert (res == NULL); > > This part should be committed regardless of the > outcome of a review of the complete patch. The > closing ')' is clearly missed placed. > OK, thanks, then I will go ahead and commit that one as obvious. Bernd.
On Wed, Sep 14, 2016 at 1:37 PM, Bernd Edlinger <bernd.edlinger@hotmail.de> wrote: > On 09/14/16 18:37, Jason Merrill wrote: >> On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger >> <bernd.edlinger@hotmail.de> wrote: >>> The other false positive was in dwarf2out, where we have this: >>> >>> ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer >>> constants in boolean context [-Werror=int-in-bool-context] >>> if (s->refcount >>> == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2)) >>> ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ >>> >>> and: >>> #define DEBUG_STR_SECTION_FLAGS \ >>> (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings \ >>> ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1 \ >>> : SECTION_DEBUG) >>> >>> which got folded in C++ to >>> if (s->refcount == >>> ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2)) >> >> Hmm, I'd think this warning should be looking at unfolded trees. >> > > Yes. The truthvalue_conversion is happening in cp_convert_and_check > at cp_convert, afterwards the cp_fully_fold happens and does this > transformation that I did not notice before, but just because I did > not have the right test case. Then if the folding did change > something the truthvalue_conversion happens again, and this time > the tree is folded potentially in a surprising way. > > The new version of the patch disables the warning in the second > truthvalue_conversion and as a consequence has to use fold_for_warn > on the now unfolded parameters. This looks like an improvement > that I would keep, because I would certainly like to warn on > x ? 1 : 1+1, which the previous version did, but just by accident. > >>> Additional to what you suggested, I relaxed the condition even more, >>> because I think it is also suspicious, if one of the branches is known >>> to be outside [0:1] and the other is unknown. >>> >>> That caused another warning in the fortran FE, which was caused by >>> wrong placement of braces in gfc_simplify_repeat. >>> >>> We have: >>> #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0) >>> >>> Therefore the condition must be .. && mpz_sgn(X) != 0) >>> >>> Previously that did not match, because -1 is outside [0:1] >>> but (Z)->size > 0 is unknown. >> >> But (Z)->size > 0 is known to be boolean; that seems like it should >> suppress this warning. > > Hmm, maybe it got a bit too pedantic, but in some cases this > warning is pointing out something that is at least questionable > and in some cases real bugs: > > For instance PR 77574, where in gcc/predict.c we had this: > > /* If edge is already improbably or cold, just return. */ > if (e->probability <= impossible ? PROB_VERY_UNLIKELY : 0 > && (!impossible || !e->count)) > return; > > thus if (x <= y ? 4999 : 0 && (...)) > >>> Furthermore the C++ test case df-warn-signedunsigned1.C also >>> triggered the warning, because here we have: >>> >>> #define MAK (fl < 0 ? 1 : (fl ? -1 : 0)) >>> >>> And thus "if (MAK)" should be written as if (MAK != 0) >> >> This also seems like a false positive to me. >> >> It's very common for code to rely on the implicit !=0 in boolean >> conversion; it seems to me that the generalization to warn about >> expressions with 0 arms significantly increases the frequency of false >> positives, making the flag less useful. The original form of the >> warning seems like a -Wall candidate to me, but the current form >> doesn't. > > Yes. The reasoning I initially had was that it is completely > pointless to have something of the form "if (x ? 1 : 2)" or > "if (x ? 0 : 0)" because the result does not even depend on x > in this case. But something like "if (x ? 4999 : 0)" looks > bogus but does at least not ignore x. > > If the false-positives are becoming too much of a problem here, > then I should of course revert to the previous heuristic again. I think we could have both, where the weaker form is part of -Wall and people can explicitly select the stronger form. Jason
... resent, because message apparently bounced. On 09/14/16 21:22, Bernd Edlinger wrote: > On 09/14/16 20:11, Jason Merrill wrote: >>> >>> Yes. The reasoning I initially had was that it is completely >>> pointless to have something of the form "if (x ? 1 : 2)" or >>> "if (x ? 0 : 0)" because the result does not even depend on x >>> in this case. But something like "if (x ? 4999 : 0)" looks >>> bogus but does at least not ignore x. >>> >>> If the false-positives are becoming too much of a problem here, >>> then I should of course revert to the previous heuristic again. >> >> I think we could have both, where the weaker form is part of -Wall and >> people can explicitly select the stronger form. >> > > > Yes, agreed. So here is what I would think will be the first version. > > It can later be extended to cover the more pedantic cases which > will not be enabled by -Wall. > > I would like to send a follow-up patch for the warning on > signed-integer shift left in boolean context, which I think > should also be good for Wall. > (I already had that feature in patch version 2 but that's meanwhile > outdated). > > > Bootstrap on x86_64-pc-linux-gnu and reg-testing not yet completed. > Is it OK for trunk when reg-testing finished? > > > Thanks > Bernd.
On 09/14/2016 12:11 PM, Jason Merrill wrote: > > I think we could have both, where the weaker form is part of -Wall and > people can explicitly select the stronger form. That's been a fairly standard way to handle this kind of thing. It works for me. Jeff
On 09/15/16 17:44, Jeff Law wrote: > On 09/14/2016 12:11 PM, Jason Merrill wrote: >> >> I think we could have both, where the weaker form is part of -Wall and >> people can explicitly select the stronger form. > That's been a fairly standard way to handle this kind of thing. It > works for me. > > Jeff The warning could for instance be more aggressive when -pedantic is in effect? Bernd.
On 09/15/2016 10:00 AM, Bernd Edlinger wrote: > On 09/15/16 17:44, Jeff Law wrote: >> On 09/14/2016 12:11 PM, Jason Merrill wrote: >>> >>> I think we could have both, where the weaker form is part of -Wall and >>> people can explicitly select the stronger form. >> That's been a fairly standard way to handle this kind of thing. It >> works for me. >> >> Jeff > > The warning could for instance be more aggressive when -pedantic is in > effect? I wouldn't do it on -pedantic. We've usually used levels or -Wfoo -Wfoo-bar kinds of schemes. jeff
On 09/15/16 18:23, Jeff Law wrote: > On 09/15/2016 10:00 AM, Bernd Edlinger wrote: >> On 09/15/16 17:44, Jeff Law wrote: >>> On 09/14/2016 12:11 PM, Jason Merrill wrote: >>>> >>>> I think we could have both, where the weaker form is part of -Wall and >>>> people can explicitly select the stronger form. >>> That's been a fairly standard way to handle this kind of thing. It >>> works for me. >>> >>> Jeff >> >> The warning could for instance be more aggressive when -pedantic is in >> effect? > I wouldn't do it on -pedantic. We've usually used levels or -Wfoo > -Wfoo-bar kinds of schemes. It would be kind of good to enable the extended warning level on the gcc bootstrap, where we have -Wall and -pedantic and more. So level 1 could be enabled with -Wall and level 2 could be enabled with -pedantic and/or -Wpedantic. Bernd.
On Thu, 15 Sep 2016, Bernd Edlinger wrote: > So level 1 could be enabled with -Wall and level 2 could be enabled > with -pedantic and/or -Wpedantic. But this warning has absolutely nothing to do with things that are prohibited by, or undefined in, ISO standards, and so it would be completely inappropriate for -pedantic to affect it in any way.
On Thu, Sep 15, 2016 at 1:42 PM, Joseph Myers <joseph@codesourcery.com> wrote: > On Thu, 15 Sep 2016, Bernd Edlinger wrote: > >> So level 1 could be enabled with -Wall and level 2 could be enabled >> with -pedantic and/or -Wpedantic. > > But this warning has absolutely nothing to do with things that are > prohibited by, or undefined in, ISO standards, and so it would be > completely inappropriate for -pedantic to affect it in any way. Agreed. We already pass several warning flags to GCC bootstrap, we can add another. Jason
On 09/15/16 21:54, Jason Merrill wrote: > On Thu, Sep 15, 2016 at 1:42 PM, Joseph Myers <joseph@codesourcery.com> wrote: >> On Thu, 15 Sep 2016, Bernd Edlinger wrote: >> >>> So level 1 could be enabled with -Wall and level 2 could be enabled >>> with -pedantic and/or -Wpedantic. >> >> But this warning has absolutely nothing to do with things that are >> prohibited by, or undefined in, ISO standards, and so it would be >> completely inappropriate for -pedantic to affect it in any way. > > Agreed. We already pass several warning flags to GCC bootstrap, we > can add another. > Yes. Maybe -Wextra could also enable the more strict warning. That option is already enabled in the bootstrap and elsewhere. Bernd.
Index: gcc/builtins.c =================================================================== --- gcc/builtins.c (revision 240135) +++ gcc/builtins.c (working copy) @@ -7887,15 +7887,18 @@ fold_builtin_classify (location_t loc, tree fndecl tree isinf_call = build_call_expr_loc (loc, isinf_fn, 1, arg); signbit_call = fold_build2_loc (loc, NE_EXPR, integer_type_node, - signbit_call, integer_zero_node); + signbit_call, integer_zero_node); isinf_call = fold_build2_loc (loc, NE_EXPR, integer_type_node, - isinf_call, integer_zero_node); + isinf_call, integer_zero_node); - tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, signbit_call, - integer_minus_one_node, integer_one_node); tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, - isinf_call, tmp, - integer_zero_node); + signbit_call, integer_minus_one_node, + integer_one_node); + /* Avoid a possible -Wint-in-bool-context warning in C. */ + tmp = fold_build2_loc (loc, PLUS_EXPR, integer_type_node, tmp, + integer_zero_node); + tmp = fold_build3_loc (loc, COND_EXPR, integer_type_node, + isinf_call, tmp, integer_zero_node); } return tmp; Index: gcc/c-family/c-common.c =================================================================== --- gcc/c-family/c-common.c (revision 240135) +++ gcc/c-family/c-common.c (working copy) @@ -4652,6 +4652,17 @@ c_common_truthvalue_conversion (location_t locatio TREE_OPERAND (expr, 0)); case COND_EXPR: + if (warn_int_in_bool_context) + { + tree val1 = fold_for_warn (TREE_OPERAND (expr, 1)); + tree val2 = fold_for_warn (TREE_OPERAND (expr, 2)); + if ((TREE_CODE (val1) == INTEGER_CST + && !integer_zerop_or_onep (val1)) + || (TREE_CODE (val2) == INTEGER_CST + && !integer_zerop_or_onep (val2))) + warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context, + "?: using integer constants in boolean context"); + } /* Distribute the conversion into the arms of a COND_EXPR. */ if (c_dialect_cxx ()) { Index: gcc/c-family/c.opt =================================================================== --- gcc/c-family/c.opt (revision 240135) +++ gcc/c-family/c.opt (working copy) @@ -545,6 +545,10 @@ Wint-conversion C ObjC Var(warn_int_conversion) Init(1) Warning Warn about incompatible integer to pointer and pointer to integer conversions. +Wint-in-bool-context +C ObjC C++ ObjC++ Var(warn_int_in_bool_context) Warning LangEnabledBy(C ObjC C++ ObjC++,Wall) +Warn for suspicious integer expressions in boolean context. + Wint-to-pointer-cast C ObjC C++ ObjC++ Var(warn_int_to_pointer_cast) Init(1) Warning Warn when there is a cast to a pointer from an integer of a different size. Index: gcc/cp/cvt.c =================================================================== --- gcc/cp/cvt.c (revision 240135) +++ gcc/cp/cvt.c (working copy) @@ -645,6 +645,7 @@ cp_convert_and_check (tree type, tree expr, tsubst { /* Avoid bogus -Wparentheses warnings. */ warning_sentinel w (warn_parentheses); + warning_sentinel c (warn_int_in_bool_context); folded_result = cp_convert (type, folded, tf_none); } folded_result = fold_simple (folded_result); Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 240135) +++ gcc/doc/invoke.texi (working copy) @@ -273,7 +273,7 @@ Objective-C and Objective-C++ Dialects}. -Wframe-larger-than=@var{len} -Wno-free-nonheap-object -Wjump-misses-init @gol -Wignored-qualifiers -Wignored-attributes -Wincompatible-pointer-types @gol -Wimplicit -Wimplicit-function-declaration -Wimplicit-int @gol --Winit-self -Winline -Wno-int-conversion @gol +-Winit-self -Winline -Wno-int-conversion -Wint-in-bool-context @gol -Wno-int-to-pointer-cast -Winvalid-memory-model -Wno-invalid-offsetof @gol -Winvalid-pch -Wlarger-than=@var{len} @gol -Wlogical-op -Wlogical-not-parentheses -Wlong-long @gol @@ -5837,6 +5837,14 @@ warning about it. The restrictions on @code{offsetof} may be relaxed in a future version of the C++ standard. +@item -Wint-in-bool-context +@opindex Wint-in-bool-context +@opindex Wno-int-in-bool-context +Warn for suspicious use of integer values where boolean values are expected, +such as conditional expressions (?:) using non-boolean integer constants in +boolean context, like @code{if (a <= b ? 2 : 3)}. +This warning is enabled by @option{-Wall}. + @item -Wno-int-to-pointer-cast @opindex Wno-int-to-pointer-cast @opindex Wint-to-pointer-cast Index: gcc/dwarf2out.c =================================================================== --- gcc/dwarf2out.c (revision 240135) +++ gcc/dwarf2out.c (working copy) @@ -2051,9 +2051,9 @@ output_loc_operands (dw_loc_descr_ref loc, int for /* Make sure the offset has been computed and that we can encode it as an operand. */ gcc_assert (die_offset > 0 - && die_offset <= (loc->dw_loc_opc == DW_OP_call2) + && die_offset <= (loc->dw_loc_opc == DW_OP_call2 ? 0xffff - : 0xffffffff); + : 0xffffffff)); dw2_asm_output_data ((loc->dw_loc_opc == DW_OP_call2) ? 2 : 4, die_offset, NULL); } Index: gcc/fortran/simplify.c =================================================================== --- gcc/fortran/simplify.c (revision 240135) +++ gcc/fortran/simplify.c (working copy) @@ -5127,7 +5127,7 @@ gfc_simplify_repeat (gfc_expr *e, gfc_expr *n) if (len || (e->ts.u.cl->length && - mpz_sgn (e->ts.u.cl->length->value.integer)) != 0) + mpz_sgn (e->ts.u.cl->length->value.integer) != 0)) { const char *res = gfc_extract_int (n, &ncop); gcc_assert (res == NULL); Index: gcc/gensupport.c =================================================================== --- gcc/gensupport.c (revision 240135) +++ gcc/gensupport.c (working copy) @@ -201,15 +201,15 @@ add_implicit_parallel (rtvec vec) #define TRISTATE_AND(a,b) \ ((a) == I ? ((b) == N ? N : I) : \ (b) == I ? ((a) == N ? N : I) : \ - (a) && (b)) + (a) == Y && (b) == Y ? Y : N) #define TRISTATE_OR(a,b) \ ((a) == I ? ((b) == Y ? Y : I) : \ (b) == I ? ((a) == Y ? Y : I) : \ - (a) || (b)) + (a) == Y || (b) == Y ? Y : N) #define TRISTATE_NOT(a) \ - ((a) == I ? I : !(a)) + ((a) == I ? I : (a) == Y ? N : Y) /* 0 means no warning about that code yet, 1 means warned. */ static char did_you_mean_codes[NUM_RTX_CODE]; Index: gcc/testsuite/c-c++-common/Wint-in-bool-context.c =================================================================== --- gcc/testsuite/c-c++-common/Wint-in-bool-context.c (revision 0) +++ gcc/testsuite/c-c++-common/Wint-in-bool-context.c (working copy) @@ -0,0 +1,32 @@ +/* PR c++/77434 */ +/* { dg-options "-Wint-in-bool-context" } */ +/* { dg-do compile } */ + +int foo (int a, int b) +{ + if (a > 0 && a <= (b == 1) ? 1 : 2) /* { dg-warning "boolean context" } */ + return 1; + + if (a > 0 && a <= (b == 2) ? 1 : 1) /* { dg-bogus "boolean context" } */ + return 2; + + if (a > 0 && a <= (b == 3) ? 0 : 2) /* { dg-warning "boolean context" } */ + return 3; + + if (a == (((b == 3) ? 1|2|4 : 1) & 2 ? 0 : 2)) /* { dg-bogus "boolean context" } */ + return 4; + + if (a <= b ? 1000-1 : 0 && (!a || !b)) /* { dg-warning "boolean context" } */ + return 5; + + if (a ? a : 1+1) /* { dg-warning "boolean context" } */ + return 6; + + if (a ? b : 0) /* { dg-bogus "boolean context" } */ + return 7; + + if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean context" } */ + return 8; + + return 0; +} Index: gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C =================================================================== --- gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C (revision 240135) +++ gcc/testsuite/g++.dg/delayedfold/df-warn-signedunsigned1.C (working copy) @@ -7,7 +7,7 @@ extern int fl; int foo (int sz) { - if (MAK) return 1; + if (MAK != 0) return 1; return 0; } Index: gcc/tree.h =================================================================== --- gcc/tree.h (revision 240135) +++ gcc/tree.h (working copy) @@ -4361,6 +4361,15 @@ extern int integer_nonzerop (const_tree); extern int integer_truep (const_tree); +/* integer_zerop_or_onep (tree x) is nonzero if X is an integer constant + of value 0 or 1. */ + +static inline int +integer_zerop_or_onep (const_tree x) +{ + return integer_zerop (x) || integer_onep (x); +} + extern bool cst_and_fits_in_hwi (const_tree); extern tree num_ending_zeros (const_tree);
Hi Jeff, it turned out, that the changed symmetric condition in the warning hit at several places, but also caused two false positives which I had to address first. I tried also building a recent glibc, which hit a false positive when using __builtin_isinf_sign in boolean context, which is used extensively by glibc. Furtunately there were zero new warnings in linux, well done Linus ;) glibc's math.h has this definition: # if __GNUC_PREREQ (4,4) && !defined __SUPPORT_SNAN__ # define isinf(x) __builtin_isinf_sign (x) and __builtin_isinf_sign(x) is internally folded as isinf(x) ? (signbit(x) ? -1 : 1) : 0 which can trigger the warning if used in a boolean context. We do not want to warn for if (isinf(x)) of course. The other false positive was in dwarf2out, where we have this: ../../gcc-trunk/gcc/dwarf2out.c:26166:35: error: ?: using integer constants in boolean context [-Werror=int-in-bool-context] if (s->refcount == ((DEBUG_STR_SECTION_FLAGS & SECTION_MERGE) ? 1 : 2)) ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~ and: #define DEBUG_STR_SECTION_FLAGS \ (HAVE_GAS_SHF_MERGE && flag_merge_debug_strings \ ? SECTION_DEBUG | SECTION_MERGE | SECTION_STRINGS | 1 \ : SECTION_DEBUG) which got folded in C++ to if (s->refcount == ((flag_merge_debug_strings ? SECTION_MERGE : 0) ? 1 : 2)) Because SECTION_MERGE = 0x08000, the condition for the warning is satisfied here, but that is only an artifact that is created by the folding of the outer COND_EXPR. Additional to what you suggested, I relaxed the condition even more, because I think it is also suspicious, if one of the branches is known to be outside [0:1] and the other is unknown. That caused another warning in the fortran FE, which was caused by wrong placement of braces in gfc_simplify_repeat. We have: #define mpz_sgn(Z) ((Z)->size < 0 ? -1 : (Z)->size > 0) Therefore the condition must be .. && mpz_sgn(X) != 0) Previously that did not match, because -1 is outside [0:1] but (Z)->size > 0 is unknown. To suppress the bogus C++ warnings I had to suppress the warning when the condition is fully folded in cp_convert_and_check and c_common_truthvalue_conversion is called for the second time. To suppress the bogus C warning on __builtin_isinf_sign I found no other way than changing the expansion of that to isinf(x) ? (signbit(x) ? -1 : 1) + 0 : 0 which is just enough to hide the inner COND_EXPR from c_common_truthvalue_conversion, but gets immediately folded away by fold_binary so it will cause no different code. Furthermore the C++ test case df-warn-signedunsigned1.C also triggered the warning, because here we have: #define MAK (fl < 0 ? 1 : (fl ? -1 : 0)) And thus "if (MAK)" should be written as if (MAK != 0) Finally, what I already wrote in my previous mail the macros in gensupport.c mix binary and ternary logic and should be cleaned up. Bootstrap and reg-test with all languages on x86_64-pc-linux-gnu. Is it OK for trunk? Thanks Bernd. gcc: 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/77434 * doc/invoke.texi: Document -Wcond-in-bool-context. * gensupport.c (TRISTATE_AND, TRISTATE_OR, TRISTATE_NOT): Fix a warning. * tree.h (integer_zerop_or_onep): New helper function. PR middle-end/77421 * dwarf2out.c (output_loc_operands): Fix an assertion. c-family: 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/77434 * c.opt (Wcond-in-bool-context): New warning. * c-common.c (c_common_truthvalue_conversion): Warn on integer constants in boolean context. cp: 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/77434 * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here. fortran: 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/77434 * simplify.c (gfc_simplify_repeat): Fix a warning. testsuite: 2016-09-14 Bernd Edlinger <bernd.edlinger@hotmail.de> PR c++/77434 * c-c++-common/Wcond-in-bool-context.c: New test. * g++.dg/delayedfold/df-warn-signedunsigned1.C: Fix a warning.