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

[Jason Merrill <jason@redhat.com>]

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