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.)
Marek
