Page MenuHomePhabricator

[clang-tidy] New checker for redundant expressions.
ClosedPublic

Authored by etienneb on Apr 22 2016, 7:21 PM.

Details

Summary

This checker finds redundant expression on both side of a binary operator.

The current implementation provide a function to check whether expressions
are equivalent. This implementation is able to recognize the common
subset encounter in C++ program. Side-effects like "x++" are not considered
to be equivalent.

There are many False Positives related to macros and to floating point
computations (detecting NaN). The checker is ignoring these cases.

Example:

if( !dst || dst->depth != desired_depth ||
    dst->nChannels != desired_num_channels ||
    dst_size.width != src_size.width ||
    dst_size.height != dst_size.height )    <<--- bug
{

Diff Detail

Event Timeline

etienneb updated this revision to Diff 54759.Apr 22 2016, 7:21 PM
etienneb retitled this revision from to [clang-tidy] New checker for redundant expressions..
etienneb updated this object.
etienneb added a reviewer: alexfh.
etienneb added a subscriber: cfe-commits.
etienneb updated this object.Apr 22 2016, 7:40 PM

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Will check catch more complicated cases, like

if ((Point1.x < Point2.x) && (Point1.x < Point2.x)) ?

Will be good idea to add such cases in test.

Please mention this check in docs/ReleaseNotes.rst (in alphabetical order).

Will check catch more complicated cases, like

if ((Point1.x < Point2.x) && (Point1.x < Point2.x)) ?

It is catching these cases.

Will be good idea to add such cases in test.

I can add it as a test, no prob. I'm never against more tests.

I have more cases implemented, but I prefer landing this piece by piece to let people review it and report false-positive.

I'm able to recognize stuff like:

x == 10 && x <= 12  (x <= 12 is redundant),  and many other cases with bitwise operations.

I need time to deal correctly with False positives. And, I'm not sure it will be part of the same matcher. Will see..

Will check catch more complicated cases, like

[..]

It is not "yet" catching these cases:

if ((Point2.x > Point1.x) && (Point1.x < Point2.x)) ?

I believe the AreEquivalentExpression should be extended, and probably llifted to utils.
Here again, I propose to do it incrementally.

An other nice extension is to catch cases like:

(x | y | x)

In this case, you need to implement the associative operators.

etienneb updated this revision to Diff 54785.Apr 23 2016, 10:48 AM

addressed eugene requests.

Tested over LLVM code, no false positives.

Two catches:
http://reviews.llvm.org/D19460
http://reviews.llvm.org/D19451

alexfh accepted this revision.Apr 26 2016, 6:22 AM
alexfh edited edge metadata.

Awesome idea!

LG with a couple of nits.

Tested over LLVM code, no false positives.

Two catches:
http://reviews.llvm.org/D19460
http://reviews.llvm.org/D19451

The second link refers to this revision ;)

clang-tidy/misc/RedundantExpressionCheck.cpp
21

This is to some degree similar to comparing llvm::FoldingSetNodeIDs created using Stmt::Profile. However it's only useful to check for identical expressions and won't work, if you're going to extend this to equivalent expressions.

docs/clang-tidy/checks/misc-redundant-expression.rst
10

Please enclose true and false in backquotes.

This revision is now accepted and ready to land.Apr 26 2016, 6:22 AM

BTW, have you seen the alpha.core.IdenticalExpr static analyzer checker?

BTW, have you seen the alpha.core.IdenticalExpr static analyzer checker?

Anna, Jordan, and whoever else is interested in the alpha.core.IdenticalExpr checker (lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp), it looks like Etienne has reinvented a large part of this checker as a clang-tidy check. I'm not sure which of these supports more cases and has more false positives (given that the alpha.core.IdenticalExpr checker was there for quite a while, but IIUC, this clang-tidy check has been tested on a huge code base with pretty good results).

A few questions to all of you:

  1. is the alpha.core.IdenticalExpr checker going to be released in the near future?
  2. is anyone actively working on it?
  3. given that Etienne seems to be planning to continue actively working on the clang-tidy analog of that static analyzer checker, are you fine to move all of this checker's (alpha.core.IdenticalExpr) functionality to clang-tidy?
  4. more generally, should we officially recommend to use clang-tidy (instead of the static analyzer) for writing AST-based checks that don't require any path-based analysis?

BTW, have you seen the alpha.core.IdenticalExpr static analyzer checker?

Anna, Jordan, and whoever else is interested in the alpha.core.IdenticalExpr checker (lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp), it looks like Etienne has reinvented a large part of this checker as a clang-tidy check. I'm not sure which of these supports more cases and has more false positives (given that the alpha.core.IdenticalExpr checker was there for quite a while, but IIUC, this clang-tidy check has been tested on a huge code base with pretty good results).

BTW, have you seen the alpha.core.IdenticalExpr static analyzer checker?

Yes, I looked at it. The AreEquivalentExpression is pretty much a variant of that code (a simplification).

A few questions to all of you:

  1. is the alpha.core.IdenticalExpr checker going to be released in the near future?
  2. is anyone actively working on it?
  3. given that Etienne seems to be planning to continue actively working on the clang-tidy analog of that static analyzer checker, are you fine to move all of this checker's (alpha.core.IdenticalExpr) functionality to clang-tidy?
  4. more generally, should we officially recommend to use clang-tidy (instead of the static analyzer) for writing AST-based checks that don't require any path-based analysis?

For now, there are overlap between both checkers. I think 'alpha.core.IdenticalExpr' is in "alpha"-mode because there are too many false-positives related to cases like macro/floating-points (detecting NaN,...). The current checker has a pretty-low false-positive ratio. But, some redundant expressions won't be reported.

i.e.   A[(3) - (3)]  which is frequent in bison-generated files won't be reported.

On a long term, I'm planning rules to enhance matching redundant expressions like:

x == 10 && x < 12   (x < 12 is useless)
etienneb updated this revision to Diff 55028.Apr 26 2016, 10:06 AM
etienneb marked 2 inline comments as done.

address alexfh comments.

clang-tidy/misc/RedundantExpressionCheck.cpp
21

Exact. There is plan to extend the notion of "equivalent".
I think we should lift this to "utils", and maybe we can also provide a "isIdenticalExpression" based on FoldingSet.

etienneb closed this revision.Apr 26 2016, 10:36 AM