Extend IdenticalExprChecker to find more potential problems

[Daniel Fahlgren]

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.

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();
  }
> 
> 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.

> Finally, the typo "comparision" is still in the patch. :-)

I should have paid more attention. Sorry :-) Attached is a new version
of the second patch.
> 
Cheers,
Daniel Fahlgren

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bin_op.patch
Type: text/x-patch
Size: 7174 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140204/658d2baa/attachment.bin>