Re: [C/C++ PATCH] RFC: Implement -Wduplicated-cond (PR c/64249)

[Marek Polacek <polacek at redhat dot com>]

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