This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [C/C++ PATCH] Implement -Wbool-compare (PR c++/62153)
- From: Manuel López-Ibáñez <lopezibanez at gmail dot com>
- To: Marek Polacek <polacek at redhat dot com>
- Cc: GCC Patches <gcc-patches at gcc dot gnu dot org>, Jason Merrill <jason at redhat dot com>, "Joseph S. Myers" <joseph at codesourcery dot com>
- Date: Tue, 19 Aug 2014 11:08:39 +0200
- Subject: Re: [C/C++ PATCH] Implement -Wbool-compare (PR c++/62153)
- Authentication-results: sourceware.org; auth=none
- References: <20140818200433 dot GD11974 at redhat dot com> <CAESRpQCKZE=Z0tqev2-aijrku5Pw50naDwJXms89BEwOrO5k8g at mail dot gmail dot com> <20140819045818 dot GF11974 at redhat dot com>
On 19 August 2014 06:58, Marek Polacek <polacek@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 <polacek@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.