Extend IdenticalExprChecker to find more potential problems

[Jordan Rose]

On Feb 5, 2014, at 11:59 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi,
> 
> On Tue, 2014-02-04 at 19:54 -0800, Jordan Rose wrote:
> 
>> Hm, you're right; the current code doesn't handle this, and perhaps
>> shouldn't (even in the non-empty case). How about this case, though?
> 
>> if (foo()) {
>> doSomething();
>> } else if (bar()) {
>> /* no action needed */
>> } else {
>> /* FIXME: Implement this. */
>> }
> 
>> This one will get flagged, correct? Should it?
> 
>> (You could argue that this isn't great code, but since it's easy to
>> special-case this I wanted to ask what you think.)
> 
> Yes it will emit a warning, and I think it should. With code written
> like this it's easy to see that the warning is harmless, or fix it. If
> you end up with two empty branches after the preprocessing the warning
> is more valid. The function call to bar() might be expensive and should
> be removed, or have side effects that must be handled in one of the
> branches. Calling bar() as condition to an if statement also indicates
> that you should run some code based on its return value, and now you
> don't.
> 
> I have added a test case for this.
> 
>>>> More test cases for the logical op checker:
>>> 
>>>> x == 4 && y == 5 || x == 4
>>>> x == 4 || y == 5 && x == 4
>>>> x == 4 || x == 4 && y == 5
>>>> 
>>>> All of these would be fair to warn on, but we can probably get
>>>> away
>>>> with not for the first pass. If we already support one, though,
>>>> should we add it as a regression test?
>>> 
>>> Unfortunately we don't support that right now, but I agree it would
>>> be a good addition.
> 
>> I would put them in anyway, then, even if there's no warning today.
> 
> Ok. I have added them to the test cases with a fixme. New patches
> attached.

Committed in r201701 and r201702!

Jordan