Re: [PATCH, updated] Add a warning for suspicious use of conditional expressions in boolean context

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

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.