On 09/17/2015 10:05 AM, Marek Polacek wrote:
The patch doesn't diagnose some more involved cases like the one
below:
if (i < 0) return 1; else if (!(i > 0 || i == 0)) return 2;
even though it does diagnose some other such cases, including:
if (i < 0) return 1; else if (!(i >= 0)) return 2;
and even:
int foo (int a, int b, int c) {
if (a + c == b) return 1; else if (a + c - b == 0) return 2;
return 0;
}
I'm guessing this is due to operand_equal_p returning true for
some pairs of equivalent expressions and false for others. Would
making this work be feasible?
You're right that this is limited by what operand_equal_p considers equal and
what not. I'm not sure if there is something more convoluted else I could use
in the FE, but I think not. It certainly doesn't look terrible important to
me.
And you'll run into a point of diminishing returns quickly. There's
many ways to write this stuff in ways that are equivalent, but a pain to
uncover. I think relying on operand_equal_p is probably sufficient at
this stage.
You probably didn't intend to diagnose the final else clause in
the following case but I wonder if it should be (as an extension
of the original idea):
if (i) return 1; else if (!i) return 2; else return 0;
Diagnosing this wasn't my intention. I'm afraid it would be kinda hard
to do that.
This is the "unreachable code" problem. We used to have a warning for
that, but in practice it was so unstable it was removed.
(In fact, I wonder if it might be worth diagnosing even the
'if (!i)' part since the condition is the inverse of the one
in the if and thus redundant).
This, on the other hand, seems doable provided we have some predicate that
would tell us whether an expression is a logical inverse of another expression.
E.g. "a > 3" and "a <= 3". Something akin to pred_equal_p -- invert one expr
and check whether they're equal.
I think you just want to select a canonical form. So you canonicalize,
then compare.
jeff
