Page MenuHomePhabricator

[clang-tidy] Adds performance-returning-type check.
AbandonedPublic

Authored by staronj on Jun 13 2016, 12:46 PM.

Details

Summary

Adds performance-returning-type check.

This check is trying to find places, where one is returning different type than function returning type and when returned object is being copied instead of moved. Check wraps it with std::move when it could make it being moved.

I ran this check on LLVM codebase and it produced following changes: http://reviews.llvm.org/D21302

Diff Detail

Event Timeline

staronj updated this revision to Diff 60587.Jun 13 2016, 12:46 PM
staronj retitled this revision from to [clang-tidy] Adds performance-returning-type check..
staronj updated this object.
staronj added reviewers: Prazek, alex.
staronj added a subscriber: cfe-commits.
Prazek added inline comments.Jun 13 2016, 1:48 PM
clang-tidy/performance/ReturningTypeCheck.cpp
43

This seems like a usefull matcher. We should probably add it to ASTMatchers, or at least move it to Utils

docs/clang-tidy/checks/performance-returning-type.rst
26

It actually becomes boost::optional<std::string>(std::move(s));

35

Please add some subsection, or something similiar with note:
"This check requires C++11 or higher to run"

35

Also describe a little bit when the checks doesn't triggers, or when it triggers, like
"for function returning A, that is constructible from B
check triggers when:

  • B::B(A &&)
  • B::B(A) when A is movable and not trivially copyable
  • A::operator B() when B is movable

check doesn't trigger when

  • B::B(const A&)
  • a is const

(and some other rules that I didn't add here)

Prazek added inline comments.Jun 13 2016, 1:52 PM
clang-tidy/performance/ReturningTypeCheck.cpp
53

Macro instructions_unclear: matches were unclear matched matches
A little bit too much used word matcher/matches :D

132

I am aware of LLVM style guide, but I think there is exception for the matchers - they looks much more like a matchers when they starts with lower case. I am not sure if it is not spoken rule, but I don't remember single binded matcher that would start with capital case.

Eugene.Zelenko added inline comments.
docs/ReleaseNotes.rst
238

One sentence should be enough. Please also remove "This check". "We" should be replaced with "function".

docs/clang-tidy/checks/performance-returning-type.rst
7

Please highlight std::move with ``. Same below

Eugene.Zelenko set the repository for this revision to rL LLVM.
mnbvmar added inline comments.Jun 14 2016, 2:11 AM
clang-tidy/performance/ReturningTypeCheck.cpp
69

Probably hasOneActiveArgument.

72

In hasParameter(N, ...), argument id is 0-based. Did you mean to match the third parameter (1-based)?

alexfh requested changes to this revision.Jun 17 2016, 5:41 AM
alexfh edited edge metadata.

Apart from the other reviewers' comments, I have a few concerns:

  1. The "performance-returning-type" name is missing an important part of information. Maybe "performance-return-value-conversion", "performance-return-value-copy", "performance-type-conversion-on-return", "performance-return-different-type" or something like that.
  2. The "expression could be wrapped with std::move" warning is close to useless. There are many things that "could be", but the main question is "why"? The first thing the message has to explain, is what's wrong with the current state, and only then suggest a solution. Also think about cases where the suggested solution is not the optimal one (e.g. changing the return type is better).
  3. What's so special about return? Looks like a more general problem is: passing an lvalue to a function/constructor/operator by const reference where it could instead be moved (the lvalue isn't used afterwards and the appropriate overload for rvalue reference is available).
This revision now requires changes to proceed.Jun 17 2016, 5:41 AM
Prazek edited edge metadata.Jun 17 2016, 7:31 AM
  1. What's so special about return? Looks like a more general problem is: passing an lvalue to a function/constructor/operator by const reference where it could instead be moved (the lvalue isn't used afterwards and the appropriate overload for rvalue reference is available).

Yes, you are right. We could generalize it more to suggest std::move for any last usage of variable (if there would be rvalue overload), but it is harde to do than this. The special thing about return, is that in most cases you shouldn't write 'return std::move(...)', so we want to find the cases where you should do it.

staronj updated this revision to Diff 61402.Jun 21 2016, 10:28 AM
staronj edited edge metadata.
staronj removed rL LLVM as the repository for this revision.
staronj marked 8 inline comments as done.
  1. Name changed to return-value-copy.
  2. Changed warning message.
  3. Fixed check description.
clang-tidy/performance/ReturningTypeCheck.cpp
132

See UnnecessaryValueParamCheck.cpp line 33 or LoopConvertCheck.cpp lines 37-47. They all start with capital letter.

Prazek added inline comments.Jun 21 2016, 11:08 AM
clang-tidy/performance/ReturnValueCopyCheck.cpp
107–108 ↗(On Diff #61402)

You have bug here, but it should not affect the check -
hasDescendant(cxxConstructorDecl()) could possibly match to constructor of another class defined inside class.

Try
cxxRecordDecl(hasMethod(cxxConstructorDecl(InnerMatcher))))

Prazek added inline comments.Jun 21 2016, 11:30 AM
clang-tidy/performance/ReturnValueCopyCheck.cpp
53 ↗(On Diff #61402)

This one is usefull AF. Can you put into Traversal AST Matchers?

Macro brilliant:

114 ↗(On Diff #61402)

maybe isConstructibleFromType? hasConstructorFromType seems like it would take cxxConstructorDecl or something.
BTW what about conversion operator? I don't see any test using it. If implementing this would be too much work, then just write FIXME for now. I guess this case would be very uncommon.

142 ↗(On Diff #61402)

dot at the end

aaron.ballman added inline comments.Jun 22 2016, 6:36 AM
clang-tidy/performance/ReturnValueCopyCheck.cpp
53 ↗(On Diff #61402)

I don't think this is a good candidate for a public AST matcher -- it does too many disparate things.

64 ↗(On Diff #61402)

With proper documentation and tests, this would be a good candidate to move into ASTMatchers.h though. However, it should be called hasDefaultArg() instead.

69 ↗(On Diff #61402)

Arguments are what the caller provides. Parameters are what the function accepts. Also "active" is a bit of a strange term for here, so I think this should be renamed to something like hasOneNonDefaultedParam() or something similar.

77 ↗(On Diff #61402)

I think we should make hasType() work for TemplateTypeParmDecl instead of doing it this way (as part of the public AST matcher interface), assuming it doesn't currently work.

86 ↗(On Diff #61402)

I think this might actually be better expressed as a local variable in registerMatchers() instead of a matcher like this.

88 ↗(On Diff #61402)

s/danger/dangerous
s/with/which

89 ↗(On Diff #61402)

s/mean/means
s/cant/can't

111–112 ↗(On Diff #61402)

Can you fix the grammatical issues with the comment? I'm not quite certain what it should say, but I think it's something like:

Matches a QualType whose declaration has a converting constructor accepting an argument of the type from the given inner matcher.

130–131 ↗(On Diff #61402)

Also ungrammatical (and missing a period at the end of the sentence).

clang-tidy/performance/ReturnValueCopyCheck.h
20 ↗(On Diff #61402)

an object of a different type.

Adding Richard who was working on standard defect report that deprived sense of this check. There are some corner cases that are not in the standard and probably won't be handled in the future that we could handle still.

alexfh requested changes to this revision.Jul 19 2016, 4:51 PM
alexfh edited edge metadata.

removing from my dashboard, while the check is waiting for Richard's comments.

This revision now requires changes to proceed.Jul 19 2016, 4:51 PM
staronj abandoned this revision.Apr 23 2017, 2:03 AM
staronj marked 9 inline comments as done.

Because of https://reviews.llvm.org/D21619 the check is no longer required.