On Wed, Sep 16, 2015 at 02:59:12PM -0600, Martin Sebor wrote:
> On 09/16/2015 09:59 AM, Marek Polacek wrote:
> >This patch implements a new warning, -Wduplicated-cond. It warns for
> >code such as
> >
> > if (x)
> > // ...
> > else if (x)
> > // ...
>
> As usual, I like this improvement. I think it's worth extending
> to conditional expressions (e.g., x ? f() : x ? g() : h() should
> be diagnosed as well).
Maybe, probably. I admit I wasn't thinking of conditional expressions
at all.
> The patch currently issues a false positive for the test case
> below. I suspect the chain might need to be cleared after each
> condition that involves a side-effect.
>
> int foo (int a)
> {
> if (a) return 1; else if (++a) return 2; else if (a) return 3;
> return 0;
> }
But the last branch here can never be reached, right? If a == 0, foo
returns 2, otherwise it just returns 1. So I think we should diagnose
this.
> 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.
> 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.
> (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.
But that sounds like another warning ;). And it smells of doing some kind of
VRP in the FE - ick.
> Is diagnosing things like 'if (a) if (a);' or 'if (a); else {
> if (a); }' not feasible or too involved, or did you choose not
> to because you expect it might generate too many warnings? (If
> the latter, perhaps it could be disabled by default and enabled
> by -Wduplicated-cond=2).
I intentionally avoided that. It would certainly be harder and I'm
not sure it's worth it. We'd need to be careful to not warn on e.g.
if (a > 5)
{
--a;
if (a < 5) // ...
}
etc.
Thanks a lot for looking into this.
Marek
