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

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

Febbe added a comment.

So I finally had time to apply your feedback.



================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:141
+      assert(BlockElement.DeclRefBlock);
+      auto Block = BlockElement.DeclRefBlock->succs();
+      // No successing DeclRefExpr found, appending successors
----------------
njames93 wrote:
> replace auto with type here.
Unused, because inlined next line.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:236
+  for (auto &Type : BlockedTypes) {
+    if (Param->getType().getAsString().find(Type) != std::string::npos) {
+      return; // The type is blocked
----------------
njames93 wrote:
> Isn't blocked types meant to be a regular expression?
It is also handled twice, therefor I just removed it.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:294
+  }
+  // Generate Diagnostic
+}
----------------
njames93 wrote:
> What's this comment for?
Removed, old Todo I have overseen.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:10
+
+#pragma once
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_UNNECESSARY_COPY_ON_LAST_USE_H
----------------
njames93 wrote:
> Remove this, the include guard is enough.
Yes, I thought, `#pragma once` is less bug prone and might be faster. 


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:21
+namespace clang {
+namespace tidy::performance {
+
----------------
njames93 wrote:
> I have no preference either way, but can these namespaces all be nested or none nested.
Had a forward declaration there, and added it again to reduce include dependencies.


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/performance/unnecessary-copy-on-last-use.cpp:3
+// RUN: %check_clang_tidy %s -std=c++11 performance-unnecessary-copy-on-last-use %t
+#include <type_traits>
+// CHECK-FIXES: #include <utility>
----------------
njames93 wrote:
> We avoid using the standard library for tests, if you need to use anything from it you have to reimplement the parts you need yourself.
Kept the `static_assert` as comment and removed the header.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205