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:



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.


Reply via email to