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

[Chris Cotter via Phabricator via cfe-commits]

ccotter added a comment.

@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,

- 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>.
- 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.
- 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 '...'`

Would you be interested in collaborating on this? I'm not sure the status of when this phab would land, but I could contribute my suggestions after, or into, this phab depending on feedback. I wrote up a few more notes on https://discourse.llvm.org/t/new-clang-tidy-check-finding-move-candidates/67252 (again, before I realized that you had started this work already).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205