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

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

Febbe added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:79
+
+static auto findDeclRefBlock(CFG const *TheCFG, DeclRefExpr const *DeclRef)
+    -> FindDeclRefBlockReturn {
----------------
njames93 wrote:
> We generally avoid trailing return types.
Ok, I'll make them non-trailing. 
Personally, I find trailing return types more readable, and they always work like expected.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:123
+          hasLHS(ignoringParenImpCasts(declRefExpr(equalsNode(DeclRef)))))),
+      Context);
+  return !Matches.empty();
----------------
njames93 wrote:
> Matching over the entire context seems pretty and a huge drain on performance, would it not make sense to just match inside the function declaration.
> Side note maybe a RecursiveASTVisitor would make more sense here in terms of performance.
> 
> Same goes for isInLambdaCapture.
How can I reduce the Context?

Also, in which terms, the RecursiveASTVisitor would make more sense here?


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:290
+
+    if (isInLambdaCapture(Param, *Result.Context)) {
+      // Lambda captures should not be fixed.
----------------
njames93 wrote:
> What's the reason for this logic, they require a different fix -
> `x` => `x(std::move(x))` 
> And a checking langopts for c++14.
> 
> Or is this because of implicit captures?
Yes, all fixes require at least c++14.
Also, implicit captures, require a different fix. 

So this is for both.
Honestly, I was too lazy to also implement some sort of lambda capture builder, which handles all of this.






================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:30
+  UnnecessaryCopyOnLastUseCheck(StringRef Name, ClangTidyContext *Context);
+  UnnecessaryCopyOnLastUseCheck(UnnecessaryCopyOnLastUseCheck &&) = delete;
+  UnnecessaryCopyOnLastUseCheck(const UnnecessaryCopyOnLastUseCheck &) = delete;
----------------
njames93 wrote:
> We typically avoid defining all the special member functions for clang tidy checks. The destructor typically only needs to be defined if it's definition can't be inside the header file.
It can't be, because I forward declared CFG, to reduce compile time a bit.
If you prefer to include the CFG's definition in the header, I can do that.

Shall I still remove the deleted special member functions?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp:16
+
+struct Copyable {
+  Copyable() = default;
----------------
njames93 wrote:
> This struct appears to be unused and has exactly the same definition as `Movable`.
No, `Movable` is not trivially capyable, because it does not define a default destructor, `Copyable` does.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205