Extend IdenticalExprChecker to find more potential problems

[Jordan Rose]

On Feb 4, 2014, at 13:05 , Daniel Fahlgren <daniel at fahlgren.se> wrote:

> Hi Jordan,
> 
> On Mon, 2014-02-03 at 18:37 -0800, Jordan Rose wrote:
>> Thanks for running the timing checks. I agree that this seems pretty
>> benign.
>> 
>> I thought of another controversial test case:
> 
>> if (foo()) {
>> doSomething();
>> } else if (bar()) {
>> /* do nothing */
>> } else if (baz()) {
>> /* do nothing */
>> } else {
>> error();
>> }
> 
>> It'd be pretty easy to just not check empty compound-statements. What
>> do you think?
> 
> I fail to understand the problem. Can you elaborate, please? If we
> rearrange the code a bit we get
> 
>  if (foo()) {
>    doSomething();
>  } else
>    if (bar()) {
>      /* do nothing */
>    } else
>      if (baz()) {
>        /* do nothing */
>      } else {
>        error();
>      }   
> 
> with no two if-then-else-branches identical. The current code doesn't
> (correctly?) warn about this.

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.)



> 
> A future addition might be to detect the same logical expressions in
> multiple nested if statements, like:
> 
>  if (i == 1) {
>    doSomething1();
>  } else if (i == 2) {
>    doSomething2();
>  } else if (i == 2) {
>    doSomething3();
>  } else {
>    error();
>  }

Yes, that would definitely be a nice check.


>> 
>> 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.

Jordan
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140204/677a8a49/attachment.html>