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

[Nathan James via Phabricator via cfe-commits]

njames93 added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:7
+//
+// Author: Fabian Keßler "Febbe" <fabian_kessler at gmx.de>
+//===----------------------------------------------------------------------===//
----------------
Author ditto.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:47
+
+constexpr auto BlockedTypesOption = "BlockedTypes";
+constexpr auto BlockedFunctionsOption = "BlockedFunctions";
----------------
These should be static


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:52
+
+constexpr bool PrintDebug = false;
+
----------------
This doesn't belong in the final version of this patch.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:59
+
+auto findDeclRefBlock(CFG const *TheCFG, DeclRefExpr const *DeclRef)
+    -> FindDeclRefBlockReturn {
----------------
Anonymous namespaces should be kept as small as possible and only used for decls that don't have another way to specify linkage. Therefore these functions should be moved out of the namespace and marked as static.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:111
+  if (!TheCFG) {
+    return createStringError(llvm::errc::bad_address,
+                             "CFG is nullptr, aborting.\n");
----------------
Is this an expected issue to run into, if not this should be seen assert. Same goes for below.
Also given no user would ever see the error message, it's probably unnecessary to create them. If the errors are situations that are expected to happen during runtime, make this function return an optional, if not just return a Usage directly.


================
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
----------------
replace auto with type here.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:143
+      // No successing DeclRefExpr found, appending successors
+      for (auto const &Succ : BlockElement.DeclRefBlock->succs()) {
+        if (Succ) { // Succ can be nullptr, if a block is unreachable
----------------
Ditto


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:214
+
+  Finder->addMatcher(traverse(TK_AsIs, stmt(anyOf(IsMoveAssingable,
+                                                  expr(IsMoveConstructible)))),
----------------
AsIs traversal is the default kind, so there's no need to specify this.


================
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
----------------
Isn't blocked types meant to be a regular expression?


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:272
+
+  clang::SourceManager &SM = *Result.SourceManager;
+  SourceLocation EndLoc = Lexer::getLocForEndOfToken(
----------------
This code can be moved inside the template check then branch as it's not used in the else or after the if.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.cpp:294
+  }
+  // Generate Diagnostic
+}
----------------
What's this comment for?


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:7
+//
+// Author: Fabian Keßler "Febbe" <fabian_kessler at gmx.de>
+//===----------------------------------------------------------------------===//
----------------
We don't include author information in files, that is kept in the got history instead.


================
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
----------------
Remove this, the include guard is enough.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:21
+namespace clang {
+namespace tidy::performance {
+
----------------
I have no preference either way, but can these namespaces all be nested or none nested.


================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:27
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance-unnecessary-copy-on-last-use.html
+
----------------



================
Comment at: clang-tools-extra/clang-tidy/performance/UnnecessaryCopyOnLastUseCheck.h:33
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
----------------
You need at least c++11 as that's when move was introduced.


================
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>
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137205