This is an archive of the discontinued LLVM Phabricator instance.

[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

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
117

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.

318

llvm::StringSet might be a more efficient choice.

324

You could use count here.

346

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.

488

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

554–555

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

test/clang-tidy/misc-redundant-expression.cpp
387

Why did you remove these test cases?

404

Same comment as above.

JonasToth added inline comments.
clang-tidy/misc/RedundantExpressionCheck.cpp
346

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
488

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

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

I would remove the formatting changes from this function.

323

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

339

the values -> the value

347–350

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

356

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

377

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

433

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

(Btw, typo: unaffective -> ineffective)

486–487

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
499–500

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

504–507

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

504–509

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

512

Spurious parens.

576–578

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?

828

Spurious newline.

845

Spurious newline.

846–847

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

860

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

clang-tidy/misc/RedundantExpressionCheck.h
20

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
576–578

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
576–578

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

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

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
377

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

433

Comment is missing a full stop at the end.

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.