Page MenuHomePhabricator

[clang-tidy] Misc redundant expression checker updated for ineffective bitwise operator expressions
ClosedPublic

Authored by barancsuk on Oct 25 2017, 6:23 AM.

Details

Summary

Redundant Expression Checker is updated to find ineffective or redundant bitwise operator expressions.

The checker targets bitwise operator expressions with a symbolic expression and an integer literal as operands, where the expression always evaluates to the same numeric constant or leaves the symbolic expression unaltered.

Examples:

  • Always evaluates to 0:
int X;
if(0 & X) return;
  • Always evaluates to ~0:
int Y;
if(Y | ~0) return;
  • The symbol is unmodified:
int Z;
Z &= ~0;

The checker matches bitwise OR, bitwise AND and their corresponding assignment operators.

Diff Detail

Repository
rL LLVM

Event Timeline

barancsuk created this revision.Oct 25 2017, 6:23 AM

I really like these additions. Found some nits otherwise looks good.

clang-tidy/misc/RedundantExpressionCheck.cpp
953 ↗(On Diff #120235)

Maybe this could be called like exprEvaluatesToConstant, since it might actually be evaluated to zero based on the other operand, right?

954 ↗(On Diff #120235)

The parens around ~Value are redundant.

959 ↗(On Diff #120235)

Maybe I would name this differently, like exprIdempotentOperation.

962 ↗(On Diff #120235)

Same as above.

1012 ↗(On Diff #120235)

Redundant parens here as well.

1026 ↗(On Diff #120235)

The ConstExprText is not quoted in the error message.

xazax.hun added inline comments.Oct 25 2017, 6:33 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
959 ↗(On Diff #120235)

Never mind, Idempotent is not the correct term here.

It'll be good idea to mention improvement in Release Notes.

barancsuk updated this revision to Diff 120948.Oct 31 2017, 2:27 AM
barancsuk marked 4 inline comments as done.

Address review comments

barancsuk added inline comments.Oct 31 2017, 2:29 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
953 ↗(On Diff #120235)

The 'not zero' in the function name means a 'bitwise not zero' or 'negated zero' (~0), which has a bit representation containing only ones.

The function itself checks whether this value is used in a bitwise OR expression as an operand, because in this case the expression always evaluates to ~0 independently from the other operand's value.

I changed the function name to exprEvaluatesToNegatedZero.

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

I would eliminate this local variable, it does not convey additional information. Also, it does not start with a capital letter.

895 ↗(On Diff #120948)

Same as above.

barancsuk updated this revision to Diff 121500.Nov 3 2017, 10:08 AM
barancsuk marked 2 inline comments as done.

Address latest review comments

xazax.hun added inline comments.Nov 6 2017, 6:15 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
142 ↗(On Diff #121500)

I would eliminate this new line.

143 ↗(On Diff #121500)

Please initialize this variable.

147 ↗(On Diff #121500)

Maybe you could ignore the casts here instead of bellow.

150 ↗(On Diff #121500)

This will dereference a null pointer (after you initialized this variable above) if the condition above fails.

505 ↗(On Diff #121500)

Maybe this is an artifact from a missing rebase. I like the original code snippet here better.

828 ↗(On Diff #121500)

The outermost parens are redundant.

832 ↗(On Diff #121500)

The outermost parens are redundant.

836 ↗(On Diff #121500)

Maybe you could rewrite this function as:

return ((Opcode == BO_Or || Opcode == BO_OrAssign) && Value == 0) ||
      ((Opcode == BO_And || Opcode == BO_AndAssign) && ~Value == 0);
882 ↗(On Diff #121500)

You could move this declaration close to its use.

docs/clang-tidy/checks/misc-redundant-expression.rst
28 ↗(On Diff #121500)

This is confusing. Maybe it would be better to have two separate example here.

barancsuk updated this revision to Diff 121744.Nov 6 2017, 9:01 AM
barancsuk marked 10 inline comments as done.

Address latest review comments

clang-tidy/misc/RedundantExpressionCheck.cpp
150 ↗(On Diff #121500)

The entire function ignoreCastsAndCopyConstructorCalls is unnecessary, since when the test cases with operators that take arguments by value were removed. My bad! I eliminated the function.

xazax.hun accepted this revision.Nov 7 2017, 5:00 AM

LGTM! But wait one more reviewer to be sure :)

This revision is now accepted and ready to land.Nov 7 2017, 5:00 AM
aaron.ballman added inline comments.Nov 15 2017, 9:41 AM
clang-tidy/misc/RedundantExpressionCheck.cpp
519 ↗(On Diff #121744)

Spurious formatting change?

686–687 ↗(On Diff #121744)

No real need for these locals, is there?

Also, having "ineff-bitwise" along with "ineffective-bitwise" is a bit odd. Can you pick a more clearly distinct name?

barancsuk updated this revision to Diff 123343.Nov 17 2017, 7:49 AM
barancsuk marked 2 inline comments as done.

Address review comments

This patch needs to be rebased off trunk before it can be applied cleanly.

This patch needs to be rebased off trunk before it can be applied cleanly.

https://reviews.llvm.org/D39243 is set as a dependency, so I suspect this can be applied cleanly once https://reviews.llvm.org/D39243 landed.

This patch needs to be rebased off trunk before it can be applied cleanly.

https://reviews.llvm.org/D39243 is set as a dependency, so I suspect this can be applied cleanly once https://reviews.llvm.org/D39243 landed.

I have to admit that this stack of interdependent changes makes it *very* hard to effectively review what's going on.

:( My initial intention was to simplify the review process by splitting the change-set into small patches, but it seems that this idea fell flat...

:( My initial intention was to simplify the review process by splitting the change-set into small patches, but it seems that this idea fell flat...

I could not apply the patch after commiting the dependent revision. Could you rebase this on top of tree clang tidy?

alexfh requested changes to this revision.Dec 6 2017, 12:10 PM
alexfh added inline comments.
clang-tidy/misc/RedundantExpressionCheck.cpp
871 ↗(On Diff #123343)

This can be a StringRef to avoid unnecessary copies. Same above.

875 ↗(On Diff #123343)

I'd use clang diagnostic formatting facilities here to avoid unnecessary temporary strings:

diag(Loc, "expression ... '%0'") << ExprText;

Same above.

This revision now requires changes to proceed.Dec 6 2017, 12:10 PM
barancsuk updated this revision to Diff 126377.Dec 11 2017, 8:46 AM
barancsuk edited edge metadata.
barancsuk marked 2 inline comments as done.

Address review comments

This revision was not accepted when it landed; it landed in state Needs Review.Dec 20 2017, 4:23 AM
This revision was automatically updated to reflect the committed changes.

There was two accepts and the comments found by Alex were resolved, so I went ahead committing this. In case there are further comments, we can address them separately.