On Wed, Sep 14, 2016 at 1:37 PM, Bernd Edlinger <bernd.edlin...@hotmail.de> wrote: > On 09/14/16 18:37, Jason Merrill wrote: >> On Wed, Sep 14, 2016 at 12:10 PM, Bernd Edlinger >> <bernd.edlin...@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