Page MenuHomePhabricator

[clang-tidy] New check `misc-redundant-condition`
Needs ReviewPublic

Authored by baloghadamsoftware on Jun 5 2020, 9:20 AM.

Details

Summary

Checking the same condition again in a nested if usually make no sense, except if the value of the expression could have been changed between the two checks. Although compilers may optimize this out, such code is suspicious: the programmer may have meant to check something else. Therefore it is worth to find such places in the code and notify the user about the problem.

This patch implements a basic check for this problem. Currently it only detects redundant conditions where the condition is a variable of integral type. It also detects the possible bug if the variable is in an "or" or "and" logical expression in the inner if and/or the variable is in an "and" logical expression in the outer if statement. Negated cases are not handled yet.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJun 5 2020, 9:20 AM
baloghadamsoftware marked an inline comment as done.Jun 5 2020, 9:25 AM

This check was made upon user request. I think it is a good base that can later be extended also for the negated cases (where the code inside the inner if never executes). That case is even more suspicious. I put it into misc because it is not such a bug that it should go into bugprone but rather a suspicious thing. The type of the bug is somewhat similar to the bugs detected by misc-redundant-expression so I think misc is the best place also for this one.

clang-tools-extra/docs/clang-tidy/checks/list.rst
136

What are these changes? I surely did not make them? Maybe the check adder Python script?

baloghadamsoftware retitled this revision from [Clang-Tidy] New check `misc-redundant-condition` to [clang-tidy] New check `misc-redundant-condition`.Jun 5 2020, 9:27 AM

I feel the refactoring of Aliasing should be in its own PR, with this being a child of it.

clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
39

Small nit, but using string literals for bound nodes is error prone. may I suggest defining some static strings and using those to prevent any typos.

40

Recursive matchers are a pain, but this will miss:

if (A && B && ... Y && Z)
64

This logic can be moved to the matcher.

varDecl(anyOf(parmVarDecl(), hasLocalStorage()))
68

Ditto:

varDecl(hasType(isVolatileQualified()))
72

Ditto, you get the point

88–89

use llvm::dyn_cast.

98

Again CharSourceRange::getTokenRange(Body->getEndLoc())

110

use llvm::dyn_cast.

116

That's because you are getting the character range, if you get the token range it'll get the correct range: CharSourceRange::getTokenRange

clang-tools-extra/docs/clang-tidy/checks/list.rst
136

Yeah, just undo those unrelated changes, maybe one day the python script will get sorted

Eugene.Zelenko added inline comments.
clang-tools-extra/docs/ReleaseNotes.rst
78

Please keep alphabetical order in new checks list.

81

Please use double back-ticks for language constructs. Same in documentation.

Updated according to the comments.

baloghadamsoftware marked 11 inline comments as done.Jun 8 2020, 7:33 AM
baloghadamsoftware added inline comments.
clang-tools-extra/clang-tidy/misc/RedundantConditionCheck.cpp
40

I suppose that for this purpose we have to make our own matcher? Right? This also makes the fixits more complex. I think this could be done in a later phase. I added a FIXME now.

Eugene.Zelenko added inline comments.Jun 8 2020, 7:45 AM
clang-tools-extra/docs/clang-tidy/checks/misc-redundant-condition.rst
7

Please use double back-ticks for language constructs. Same below.

These test cases don't show it, from what i can see this will erroneously match:

if (onFire) {
  tryPutFireOut();
} else {
  if (isHot && onFire) {
    scream();
  }
}

It appears that this will change the second if to:

if (isHot) {
  scream();
}

Rather than simply deleting the if as it will always evaluate to false. To fix this you could wrap the hasDescendant matcher inside a hasThen matcher.

Another case is if the condition variable is on the RHS of a redundant inner if condition, you can't delete the inner if when the LHS potentially has side effects.

if (onFire) {
  if (callTheFD() || onFire) {
    scream();
  }
}

If callTheFD has side effects this can't be replaced with this:

if (onFire) {
  scream();
}

You could however replace it with:

if (onFire) {
  callTheFD();
  scream();
}

For this simply check if the LHS has side effects.

Thank you for your help, @njames93! I updated the patch according to your comments. (And of course, also thank you, @Eugene.Zelenko, I also incorporated the changes you suggested.)

I tested this check on several open-source projects: BitCoin, CURL, OpenSSL, PostGreS/, TMux/ and Xerces. I only got two warnings issued by this checker, the first one in BitCoin:

src/checkqueue.h:93:25: redundant condition 'fMaster' [misc-redundant-condition]
                        if (fMaster)
                        ^

And indeed, this was a true positive that is fixed by now by this Commit. (I do not update these projects very often since I only use them for testing checks.)

The other one is in PostGreS:

src/backend/access/transam/xlog.c:7231:6: redundant condition 'switchedTLI' [misc-redundant-condition]
          if (switchedTLI && AllowCascadeReplication())
          ^

Althought the line numbers are changed by now, the bug is still there.

Thus the result is zero false positives and two true positives so far.

As a future work, when something support ifs, it should also support ?:, and eliminate redundant conditionals from there, too.

I believe this check (together with misc-redundant-expr) should go into the readability- group.

True positive confirmed in PostGreS: Mailing List Archive. It is fixed by now, so you cannot see it on the link in my previous comment anymore.