On 09/19/16 21:17, Jason Merrill wrote:
> On 09/15/2016 05:19 AM, Bernd Edlinger wrote:
>> + 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
>> + && TREE_CODE (val2) == INTEGER_CST
>> + && !integer_zerop (val1)
>> + && !integer_zerop (val2)
>> + && (!integer_onep (val1)
>> + || !integer_onep (val2)))
>> + warning_at (EXPR_LOCATION (expr), OPT_Wint_in_bool_context,
>> + "?: using integer constants in boolean context");
>
> I think this message could be more helpful; we're warning about code
> that always evaluates to 'true', so we could say something like
> "conditional expression always evaluates to true". In which case maybe
> your original warning name was better.
>
Yes. It is difficult to choose a good name.
I would also like to warn on signed_int shift left in boolean context,
as a follow-up, because it found also a wrong code in
gcc/cp/parser.c (cp_parser_condition) where we have:
bool flags = LOOKUP_ONLYCONVERTING;
but LOOKUP_ONLYCONVERTING is (1 << 2)
This has nothing to do with conditions,
but just with undefined integer operations in boolean context.
So I thought, these warnings would have something in common.
> I'm also not sure we need to check integer_onep at all before giving
> that warning. Were you seeing a lot of false positives without that check?
>
I did not try. This warning is trying to identify code which is
doing funny things because one of the conditional expressions are not
boolean, thus not 0 or 1. That can happen because either brackets
are there, but at the wrong place, or brackets are not there and the
precedence of ?: and && or || result in wrong code. Chances are that
"? 2 : 3" is not right in a boolean context. The fact that the
result used as a boolean is always true, adds just another point to
the score. So we have a two reasons why we would warn here,
that justifies -Wall, otherwise only -Wextra I would say.
My reasoning was that if both sides evaluate to 1 or both evaluate to
0, it may well be possible that we have different defines here, maybe
just configure options that are only both 1 by chance.
However there is PR 64279 which should handle exactly this:
[Bug c/64279] Warning missing for "(cond) ? A : A" / if(cond) expr1;
else expr1; // same expression in if and else branch
I think Marek is already working on it.
Thanks
Bernd.
