Re: [C/C++ PATCH] Implement -Wbool-compare (PR c++/62153)

[Manuel López-Ibáñez]

On 19 August 2014 06:58, Marek Polacek <pola...@redhat.com> wrote:
> On Mon, Aug 18, 2014 at 10:57:58PM +0200, Manuel López-Ibáñez wrote:
>> On 18 August 2014 22:04, Marek Polacek <pola...@redhat.com> wrote:
>> > +void
>> > +maybe_warn_bool_compare (location_t loc, enum tree_code code, tree op0,
>> > +                        tree op1)
>> > +{
>> > +  if (TREE_CODE_CLASS (code) != tcc_comparison)
>> > +    return;
>> > +
>> > +  /* <boolean> CMP <cst> */
>> > +  if (TREE_CODE (op1) == INTEGER_CST
>> > +      && !integer_zerop (op1)
>> > +      && !integer_onep (op1))
>> > +    {
>> > +      if (code == EQ_EXPR
>> > +         || ((code == GT_EXPR || code == GE_EXPR)
>> > +             && tree_int_cst_sgn (op1) == 1)
>> > +         || ((code == LT_EXPR || code == LE_EXPR)
>> > +             && tree_int_cst_sgn (op1) == -1))
>> > +       warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
>> > +                   "with boolean expression is always false", op1);
>> > +      else
>> > +       warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
>> > +                   "with boolean expression is always true", op1);
>> > +    }
>> > +  /* <cst> CMP <boolean> */
>> > +  else if (TREE_CODE (op0) == INTEGER_CST
>> > +          && !integer_zerop (op0)
>> > +          && !integer_onep (op0))
>> > +    {
>> > +      if (code == EQ_EXPR
>> > +         || ((code == GT_EXPR || code == GE_EXPR)
>> > +             && tree_int_cst_sgn (op0) == -1)
>> > +         || ((code == LT_EXPR || code == LE_EXPR)
>> > +             && tree_int_cst_sgn (op0) == 1))
>> > +       warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
>> > +                   "with boolean expression is always false", op0);
>> > +      else
>> > +       warning_at (loc, OPT_Wbool_compare, "comparison of constant %qE "
>> > +                   "with boolean expression is always true", op0);
>> > +    }
>>
>> Perhaps you can save some repetition by doing first:
>>
>> tree op = (TREE_CODE (op1) == INTEGER_CST) ? op1 : op0;
>> if (TREE_CODE (op) == INTEGER_CST
>>     && !integer_zerop (op)
>
> Not sure about that: it matters whether the CST is a LHS or a RHS
> - because we want to say if the comparison is always true or false.
> I tried to introduce some bool flag, but that didn't really help
> readability IMHO.  (The tree_int_cst_sgn is compared to 1 and -1, or
> to -1 and 1.)

Oh, yes. I missed that. Sorry. What about?

tree op = (TREE_CODE (op0) == INTEGER_CST) ? op0
           : (TREE_CODE (op1) == INTEGER_CST) ? op1 : NULL_TREE;

if (op == NULL_TREE)
return;

if (!integer_zerop (op) && !integer_onep(op))
{
      int sign = (TREE_CODE (op0) == INTEGER_CST)
                 ? tree_int_cst_sgn (op) : -tree_int_cst_sgn (op);
      if (code == EQ_EXPR
         || ((code == GT_EXPR || code == GE_EXPR)
             && sign < 0)
         || ((code == LT_EXPR || code == LE_EXPR)
             && sign > 0))

or some variation of the above could work, no?

Cheers,

Manuel.