Page MenuHomePhabricator

[clang-tidy] Misc redundant expressions checker updated for macros
ClosedPublic

Authored by barancsuk on Oct 9 2017, 7:13 AM.

Details

Summary

Redundant Expression Checker is updated to be able to detect expressions that contain macro constants. Also, other small details are modified to improve the current implementation.

The improvements in detail are as follows:

1.) Binary and ternary operator expressions containing two constants, with at least one of them from a macro, are detected and tested for redundancy.

Macro expressions are treated somewhat differently from other expressions, because the particular values of macros can vary across builds. They can be considered correct and intentional, even if macro values equal, produce ranges that exclude each other or fully overlap, etc. The correctness of a macro expression is decided based on solely the operators involved in the expression.

Examples:

The following expression is always redundant, independently of the macro values, because the subexpression containing the larger constant can be eliminated without changing the meaning of the expression:

(X < MACRO && X < OTHER_MACRO)

On the contrary, the following expression is considered meaningful, because some macro values (e.g.: MACRO = 3, OTHER_MACRO = 2) produce a valid interval:

(X < MACRO && X > OTHER_MACRO)

2.) The code structure is slightly modified: typos are corrected, comments are added and some functions are renamed to improve comprehensibility, both in the checker and the test file. A few test cases are moved to another function.

3.) The checker is now able to detect redundant CXXFunctionalCastExprs as well (it matched only CStyleCastExprs before). A corresponding test case is added.

Diff Detail

Repository
rL LLVM

Event Timeline

barancsuk created this revision.Oct 9 2017, 7:13 AM
Eugene.Zelenko retitled this revision from Misc redundant expressions checker updated for macros to [clang-tidy] Misc redundant expressions checker updated for macros.Oct 9 2017, 7:16 AM
Eugene.Zelenko set the repository for this revision to rL LLVM.

Thanks for working on this, these additions look very useful to me.

clang-tidy/misc/RedundantExpressionCheck.cpp
124 ↗(On Diff #118186)

You could merge this case with the above case if you cast Left and Right to ExplicitCastExpr. So both label could end up executing the same code.

330 ↗(On Diff #118186)

llvm::StringSet might be a more efficient choice.

336 ↗(On Diff #118186)

You could use count here.

357 ↗(On Diff #118186)

I think you could just return the pointer and return a null pointer in case it is not an integerConstantExpr. This way no compatibility overload needed.

510 ↗(On Diff #118186)

I would prefer to return a pair of pointer to be returned to output parameters.

579 ↗(On Diff #118186)

I think a comment might be good here what do you mean by meaningful.

test/clang-tidy/misc-redundant-expression.cpp
387 ↗(On Diff #118186)

Why did you remove these test cases?

404 ↗(On Diff #118186)

Same comment as above.

JonasToth added inline comments.
clang-tidy/misc/RedundantExpressionCheck.cpp
357 ↗(On Diff #118186)

or llvm::Optional making it clear that no result is intended.

barancsuk updated this revision to Diff 119452.Oct 18 2017, 6:08 AM
barancsuk marked 8 inline comments as done.

Address review comments.

barancsuk added inline comments.Oct 18 2017, 6:20 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
510 ↗(On Diff #118186)

I think, since 'Retrieve' functions have multiple output parameters, they should remain unchanged.

'Retrieve' methods are responsible for splitting expressions into parts (e.g.: into opcode and constant integer value) and storing each part into the corresponding output parameter. Therefore it is more reasonable to return whether the operation was successful than returning one of these parameters.

test/clang-tidy/misc-redundant-expression.cpp
387 ↗(On Diff #118186)

The above test cases were not removed, just moved into the function TestLogical, as they contain only equality operators, and no relational operators.

xazax.hun accepted this revision.Oct 25 2017, 7:09 AM

LGTM! But wait for @aaron.ballman, @alexfh, or @hokein for the final word.

This revision is now accepted and ready to land.Oct 25 2017, 7:09 AM
aaron.ballman added inline comments.Oct 25 2017, 1:35 PM
clang-tidy/misc/RedundantExpressionCheck.cpp
89 ↗(On Diff #119452)

I would remove the formatting changes from this function.

331 ↗(On Diff #119452)

This should use a StringRef instead of a std::string to avoid needless copies.

347 ↗(On Diff #119452)

the values -> the value

355–358 ↗(On Diff #119452)

This can be reduced to return ConstExpr && ConstExpr->isIntegerConstantExpr(Value, *Result.Context);

364 ↗(On Diff #119452)

The * binds to the identifier, and there's an extra ;.

388 ↗(On Diff #119452)

I'd go back towards the original formulation: Match a binary operator between a symbolic expression and an integer constant expression.

445 ↗(On Diff #119452)

I understand the first part of the comment, but not the second part -- why is double negation ineffective?

(Btw, typo: unaffective -> ineffective)

499–500 ↗(On Diff #119452)

Formatting. Also, this can use const auto * instead of spelling the name twice. Actually, this should be reformulated because you do an isa<> check above. You can replace the if statement and these assignments with:

const auto *LhsBinOp = dyn_cast<BinaryOperator>(BinOp->getLHS());
const auto *RhsBinOp = dyn_cast<BinaryOperator>(BinOp->getRHS());

if (!LhsBinOp || !RhsBinOp)
  return false
524–525 ↗(On Diff #119452)

These should use cast<> instead of dyn_cast<> (you've already asserted this property and cast<> will assert anyway).

537 ↗(On Diff #119452)

Spurious parens.

560–565 ↗(On Diff #119452)

This can be combined into a single return statement and no local variables.

575–578 ↗(On Diff #119452)

This can be return LhsLoc.isMacroID() != RhsLoc.isMacroID();

633–635 ↗(On Diff #119452)

This worries me slightly for macros because those values can be overridden for different compilation targets. So the diagnostic may be correct for a particular configuration of the macros, but incorrect for another. Have you tested this against any large code bases to see what the quality of the diagnostics looks like?

917 ↗(On Diff #119452)

Spurious newline.

934 ↗(On Diff #119452)

Spurious newline.

935–936 ↗(On Diff #119452)

Please don't use auto here as the type is not explicitly spelled out.

950 ↗(On Diff #119452)

binded? Perhaps you meant bound? (Same comment applies below.)

clang-tidy/misc/RedundantExpressionCheck.h
20 ↗(On Diff #119452)

uneffective -> ineffective

barancsuk updated this revision to Diff 120820.Oct 30 2017, 7:35 AM
barancsuk marked 17 inline comments as done.

Address review comments, discard ambiguously redundant macro expressions

barancsuk added inline comments.Oct 30 2017, 7:37 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
633–635 ↗(On Diff #119452)

Thank you for your insight!

Now that I come to think of it, although these expressions are redundant for every configuration, there is no adequate simplification for each compilation target, because they might change their behavior if the values vary.

A good example is the following expression, that is always redundant regardless of the values that the macros hold.
However, the particular macro that causes the redundancy can vary from one compilation target to another.

(X < MACRO1 || X < MACRO2)

If MACRO1 > MACRO2, the appropriate simplification is (X < MACRO1 ),
and if MACRO1 < MACRO2, it is (X < MACRO2).

Even expressions, like (X < MACRO1 || X != MACRO2), that can almost always be simplified to (X != MACRO2), change their behavior, if MACRO1 is equal to MACRO2 (then they are always true), and can be considered conscious development choices.

In conclusion, I think, it might be better now to skip these expressions, and check for only the ones that contain the same macro twice, like X < MACRO && X > MACRO.

aaron.ballman added inline comments.Oct 30 2017, 9:32 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
633–635 ↗(On Diff #119452)

In conclusion, I think, it might be better now to skip these expressions, and check for only the ones that contain the same macro twice, like X < MACRO && X > MACRO.

I think that approach makes the most sense, at least initially.

xazax.hun added inline comments.Nov 2 2017, 8:36 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
41 ↗(On Diff #120820)

Is this block clang formatted? The indentation of the second line looks strange.

barancsuk updated this revision to Diff 121323.Nov 2 2017, 8:47 AM

Fix shifted line

barancsuk added inline comments.Nov 2 2017, 8:48 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
41 ↗(On Diff #120820)

It was, but the second line shifted somehow. I fixed it in the updated diff.

barancsuk marked 2 inline comments as done.Nov 3 2017, 9:16 AM
aaron.ballman accepted this revision.Nov 3 2017, 12:21 PM

Aside from some minor commenting nits, this LGTM.

clang-tidy/misc/RedundantExpressionCheck.cpp
445 ↗(On Diff #119452)

Comment is missing a full stop at the end.

377 ↗(On Diff #121323)

This commenting change is incorrect, the comment should end with a full stop.

barancsuk updated this revision to Diff 121740.Nov 6 2017, 8:19 AM
barancsuk marked 2 inline comments as done.

Address review comments: fix missing full stops at the end of comments

This revision was automatically updated to reflect the committed changes.