[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

[Fabian Keßler via Phabricator via cfe-commits]

Febbe added a comment.

In D137205#4018548 <https://reviews.llvm.org/D137205#4018548>, @ccotter wrote:

> @Febbe - Really glad to see this phab up! Not having realized this phab was in progress, I implemented this a few months back (though, originally not as a clang-tidy tool and never published) and thought it'd be worth sharing notes. I recently turned my non-clang-tidy implementation into a clang-tidy one here <https://github.com/llvm/llvm-project/compare/main...ccotter:llvm-project:suggest-move>. A couple suggestions based on my experience writing a similar tool,

Hello @ccotter cool, you found this phab, and that you also had the same idea. 
I would like it, to collaborate with you. I just don't have a clue how to do this properly and clean via phabricator. What's your suggestion?

> - Should we detect "escaped" values, i.e., values who have their address taken, and not apply a FixIt in these cases? See this test <https://github.com/llvm/llvm-project/compare/main...ccotter:llvm-project:suggest-move?expand=1#diff-8ba1266605f2855394ef80dd6e13b845985116ed389431d48018c7dc483bf2eeR121>.

Currently, this check does not care, if a value is taken by reference or pointer, not even when the value is taken in the same scope.
This is in fact an open Todo.
A rule of thumb would be: if the called function is analyzable, analyze it. Else, declare the move safety as unsecure and do warn, if the user is interested in it.

Generally I think, that fix it's should be always displayed, when they produce compilable code. But the move safety should be displayed in the error message.
Of course, it would be good to disable questionable fixups for automatic fix up runs. But I didn't heart of a solution to differ in a clang-tidy check, between automatic fixups and those used in an IDE.

> - I took a slightly more conservative approach in only moving values whose `QualType.isTrivialType()` is true, which addresses the `shared_ptr<lock_guard>` problem. We can then apply FixIts for containers (std::vector etc) and algebraic types (tuple/optional) of said types (and containers of containers of ...). In general, the approach I was aiming for was to define a classification of "safe to move types" (inferred from isTrivialType, and/or using a list of safe types of in the tool configuration as you have done). Containers/algebraic types of safe to move types, or smart pointers of safe to move types (being careful with any custom allocator which might lead to a similar problem as the scope guard case) are also safe to move, etc. Having the tool be able to distinguish "safe to move" FixIts from "potentially unsafe" (as a tool mode, potentially with "safe" as the default) would ensure safe refactoring.

But moving trivial types are equal to a copy, aren't they? Therefore, I search for types where this is false. 
I also think, because clang-tidy does not claim to be 100% correct, in contrast to a compiler which must be, having such cases like `shared_ptr<lock_guard>` is acceptable. Mostly such cases also have a huge smell (store mutex along with the data etc...).

> - This might be a nitpick, but in the diagnostic message `warning: Parameter '...'`, "Parameter" typically means something more specific (the names given to function variables, aka, function parameters). How about `warning: Value '...'`

Thank you for the catch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D137205/new/

https://reviews.llvm.org/D137205