Page MenuHomePhabricator

[clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved.
ClosedPublic

Authored by flx on May 15 2016, 3:45 PM.

Details

Summary

Make check more useful in the following two cases:

  1. The parameter is passed by non-const value, has a non-deleted move constructor and is only referenced once in the function as argument to the type's copy constructor.
  2. The parameter is passed by non-const value, has a non-deleted move assignment operator and is only referenced once in the function as argument of the the type's copy assignment operator.

In this case suggest a fix to move the parameter which avoids the unnecessary copy and is closest to what the user might have intended.

Diff Detail

Repository
rL LLVM

Event Timeline

flx updated this revision to Diff 57309.May 15 2016, 3:45 PM
flx retitled this revision from to [clang-tidy] UnnecessaryValueParamCheck - suggest std::move() if non-const value parameter can be moved..
flx updated this object.
flx added reviewers: alexfh, sbenza.
flx set the repository for this revision to rL LLVM.
flx added a subscriber: cfe-commits.
sbenza added inline comments.May 16 2016, 12:50 PM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
83 ↗(On Diff #57309)

We should not call this more than once as it is a potentially expensive operation.
That is, isOnlyUsedAsConst will do it too.
Also, in this case we don't care about _all_. We only care about != 1.

144 ↗(On Diff #57309)

Prefer two additions over a replacement.
They usually behave better in some conditions.

clang-tidy/utils/DeclRefExprUtils.cpp
102 ↗(On Diff #57309)

why findAll() ?
You only care about one.

same as below.

clang-tidy/utils/TypeTraits.cpp
131 ↗(On Diff #57309)

maybe also check that it is not trivial?

below too.

alexfh added inline comments.May 18 2016, 6:56 AM
clang-tidy/performance/UnnecessaryValueParamCheck.h
30 ↗(On Diff #57309)

s/clang:://

flx updated this revision to Diff 57620.May 18 2016, 7:29 AM
flx removed rL LLVM as the repository for this revision.
flx marked 4 inline comments as done.

Thanks for the feedback!

flx added inline comments.May 18 2016, 7:31 AM
clang-tidy/utils/TypeTraits.cpp
131 ↗(On Diff #57620)

Sorry I missed this. Will address it in the next revision.

flx updated this revision to Diff 58090.May 23 2016, 6:58 AM
flx added inline comments.
clang-tidy/utils/TypeTraits.cpp
131 ↗(On Diff #58090)

Done. Shortened this now to just check whether the type has a non-trivial move constructor or assignment operator. For the test to work I had to make sure the constructors/operators there are also non-trivial.

flx updated this revision to Diff 58093.May 23 2016, 7:21 AM

Upated documentation as well.

Prazek added a subscriber: Prazek.May 23 2016, 7:47 AM

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

flx added a comment.May 23 2016, 8:05 AM

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

Thanks! I'm not sure this fits with what a user would expect from a check named "unnecessary-value-param" since in this case the parameter is not passed by value. Would a separate check make sense for this?

flx added a comment.May 26 2016, 7:37 AM

This is ready for review again.

flx added a comment.May 30 2016, 5:39 PM

Alex, Sam, could you take another look? It'd be great to get this change in to make the check more useful.

In D20277#436725, @flx wrote:

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

Thanks! I'm not sure this fits with what a user would expect from a check named "unnecessary-value-param" since in this case the parameter is not passed by value. Would a separate check make sense for this?

Consider changing the name :) When I was thinking about the check for rvalue references, I wanted to name it "*-rvalue-ref-one-use".
If it covers values also, maybe "performance-move-single-used-variable" or "performance-variable-one-use", or "performance-disposable-object" (If I translated it correctly).

In my opinion there is no need to have separate check to to this, because the user would like to have both or none.

flx added a comment.May 31 2016, 7:58 AM
In D20277#436725, @flx wrote:

Cool check! Did you think about sugesting std::move for rvalue references if they are used once?

Thanks! I'm not sure this fits with what a user would expect from a check named "unnecessary-value-param" since in this case the parameter is not passed by value. Would a separate check make sense for this?

Consider changing the name :) When I was thinking about the check for rvalue references, I wanted to name it "*-rvalue-ref-one-use".
If it covers values also, maybe "performance-move-single-used-variable" or "performance-variable-one-use", or "performance-disposable-object" (If I translated it correctly).

In my opinion there is no need to have separate check to to this, because the user would like to have both or none.

Renaming the check is a breaking change since it affects user's configurations. I'm working in a code base that doesn't allow r-value references outside of move constructors and assignment, but if you'd like to add handing the r-value reference case, that'd be great! I agree that with this change it make sense to handle r-value references here too.

sbenza added inline comments.May 31 2016, 8:18 AM
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
34 ↗(On Diff #58093)

isSubset?

clang-tidy/utils/Matchers.cpp
16 ↗(On Diff #58093)

Prefer AST_MATCHER_FUNCTION.
That macro will make sure to use a singleton matcher instead, since it has no arguments.

clang-tidy/utils/Matchers.h
44 ↗(On Diff #58093)

isReferenceToConst

flx updated this revision to Diff 59157.May 31 2016, 5:11 PM
flx marked 2 inline comments as done.
flx added inline comments.
clang-tidy/performance/UnnecessaryValueParamCheck.cpp
34 ↗(On Diff #58093)

In the caller code it seems to me more intuitive to say we're checking whether the set difference is empty. But no particularly strong feelings if you think isSubset is more clear.

flx updated this revision to Diff 59167.May 31 2016, 8:08 PM
alexfh edited edge metadata.Jun 17 2016, 5:14 AM

A few nits. Otherwise looks good, if Sam has no objections.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
34 ↗(On Diff #59167)

+1 to isSubset. And please add a comment explaining what should be a subset of what for this function to return true.

clang-tidy/utils/Matchers.h
50 ↗(On Diff #59167)

How about using ast_matchers; in this function to make the statement easier to read?

clang-tidy/utils/TypeTraits.cpp
129 ↗(On Diff #59167)

Maybe simplify this a bit?

return Record && Record->hasDefinition() && Record->hasNonTrivialMoveAssignment();
136 ↗(On Diff #59167)

ditto

clang-tidy/utils/TypeTraits.h
32 ↗(On Diff #59167)

Please use doxygen comments (///) and enclose inline code snippets in backquotes. The other function comments in this file should also be doxygen, I've fixed them in r272994.

alexfh accepted this revision.Jun 17 2016, 5:14 AM
alexfh edited edge metadata.
This revision is now accepted and ready to land.Jun 17 2016, 5:14 AM
flx updated this revision to Diff 62524.Jul 1 2016, 12:40 PM
flx edited edge metadata.
flx marked 5 inline comments as done.

Thanks for the review. I addressed all remaining comments.

clang-tidy/performance/UnnecessaryValueParamCheck.cpp
34 ↗(On Diff #59167)

Omitted the comment by making the parameter names very explicit.

clang-tidy/utils/Matchers.h
50 ↗(On Diff #59167)

Used using namespace ast_matchers;.

This revision was automatically updated to reflect the committed changes.