On 09/19/16 21:51, Jeff Law wrote:
> On 09/15/2016 03:19 AM, 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 and reg-testing on x86_64-pc-linux-gnu.
>> Is it OK for trunk?
>>
>>
>> Thanks
>> Bernd.
>>
>>
>> changelog-pr77434v3.txt
>>
>>
>> gcc:
>> 2016-09-14 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>
>> PR c++/77434
>> * doc/invoke.texi: Document -Wcond-in-bool-context.
>>
>> PR middle-end/77421
>> * dwarf2out.c (output_loc_operands): Fix an assertion.
>>
>> c-family:
>> 2016-09-14 Bernd Edlinger <bernd.edlin...@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.edlin...@hotmail.de>
>>
>> PR c++/77434
>> * cvt.c (cp_convert_and_check): Suppress Wint-in-bool-context here.
>>
>> testsuite:
>> 2016-09-14 Bernd Edlinger <bernd.edlin...@hotmail.de>
>>
>> PR c++/77434
>> * c-c++-common/Wcond-in-bool-context.c: New test.
>>
>>
>> patch-pr77434v3.diff
>>
>>
>> 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;
> This hunk is not mentioned in the ChangeLog and there's a good chance
> this has or is going to change significantly. I don't like the tmp+0
> workaround either. If there isn't an immediate need, can we let this
> hunk slide and come back to it after the other changes from Tamar &
> Wilco are wrapped up?
>
Yes that would be good.
> I think this is OK with the builtins.c hunk dropped as long as exclusion
> of that hunk doesn't trigger spurious warnings.
>
It does trigger a warning but it is usually masked by -Wsystem-headers,
so I initially missed it completely, but it comes quite often when
I build a recent glibc. That does only happen with C, not C++.
If I drop that chunk then I must also drop that line
if (__builtin_isinf_sign ((float)a/b)) /* { dg-bogus "boolean
context" } */
But I have to come back and silence the warning on that construct
in a follow-up patch.
If it is OK, knowing it is work in progress,
I could hold back the builtins part for now,
and commit what I have ?
Thanks
Bernd.
