[clang-tools-extra] [clang-tidy] Add `performance-explicit-move-constructor` check (PR #122599)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: dodicidodici (dodicidodici)

<details>
<summary>Changes</summary>

This patch adds a check that reports classes that define an explicit move constructor and a copy constructor.
Closes #<!-- -->121707 

---
Full diff: https://github.com/llvm/llvm-project/pull/122599.diff


8 Files Affected:

- (modified) clang-tools-extra/clang-tidy/performance/CMakeLists.txt (+1) 
- (added) clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp (+73) 
- (added) clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h (+33) 
- (modified) clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp (+3) 
- (modified) clang-tools-extra/docs/ReleaseNotes.rst (+6) 
- (modified) clang-tools-extra/docs/clang-tidy/checks/list.rst (+1) 
- (added) clang-tools-extra/docs/clang-tidy/checks/performance/explicit-move-constructor.rst (+33) 
- (added) clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp (+34) 


``````````diff
diff --git a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
index c6e547c5089fb0..07f4e84b00bb3e 100644
--- a/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -6,6 +6,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_library(clangTidyPerformanceModule STATIC
   AvoidEndlCheck.cpp
   EnumSizeCheck.cpp
+  ExplicitMoveConstructorCheck.cpp
   FasterStringFindCheck.cpp
   ForRangeCopyCheck.cpp
   ImplicitConversionInLoopCheck.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
new file mode 100644
index 00000000000000..94f9cebae69506
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
@@ -0,0 +1,73 @@
+//===--- ExplicitMoveConstructorCheck.cpp - clang-tidy --------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "ExplicitMoveConstructorCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::performance {
+
+static SourceRange findExplicitToken(const CXXConstructorDecl *Ctor,
+                                     const SourceManager &Source,
+                                     const LangOptions &LangOpts) {
+  SourceLocation CurrentLoc = Ctor->getBeginLoc();
+  SourceLocation EndLoc = Ctor->getEndLoc();
+  Token Tok;
+
+  do {
+    const bool failed = Lexer::getRawToken(CurrentLoc, Tok, Source, LangOpts);
+
+    if (failed)
+      return {};
+
+    if (Tok.is(tok::raw_identifier) && Tok.getRawIdentifier() == "explicit")
+      return {Tok.getLocation(), Tok.getEndLoc()};
+
+    CurrentLoc = Tok.getEndLoc();
+  } while (Tok.isNot(tok::eof) && CurrentLoc < EndLoc);
+
+  return {};
+}
+
+void ExplicitMoveConstructorCheck::registerMatchers(MatchFinder *Finder) {
+  Finder->addMatcher(
+      cxxRecordDecl(
+          has(cxxConstructorDecl(isMoveConstructor(), isExplicit(),
+                                 unless(isDeleted()))
+                  .bind("move-ctor")),
+          has(cxxConstructorDecl(isCopyConstructor(), unless(isDeleted()))
+                  .bind("copy-ctor"))),
+      this);
+}
+
+void ExplicitMoveConstructorCheck::check(
+    const MatchFinder::MatchResult &Result) {
+  const auto *MoveCtor =
+      Result.Nodes.getNodeAs<CXXConstructorDecl>("move-ctor");
+  const auto *CopyCtor =
+      Result.Nodes.getNodeAs<CXXConstructorDecl>("copy-ctor");
+
+  if (!MoveCtor || !CopyCtor)
+    return;
+
+  auto Diag =
+      diag(MoveCtor->getLocation(),
+           "copy constructor may be called instead of move constructor");
+  SourceRange ExplicitTokenRange =
+      findExplicitToken(MoveCtor, *Result.SourceManager, getLangOpts());
+
+  if (ExplicitTokenRange.isInvalid())
+    return;
+
+  Diag << FixItHint::CreateRemoval(
+      CharSourceRange::getCharRange(ExplicitTokenRange));
+}
+
+} // namespace clang::tidy::performance
diff --git a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
new file mode 100644
index 00000000000000..9ae071a20a9eef
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
@@ -0,0 +1,33 @@
+//===--- ExplicitMoveConstructorCheck.h - clang-tidy ------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EXPLICITMOVECONSTRUCTORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EXPLICITMOVECONSTRUCTORCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::performance {
+
+/// Find classes that define an explicit move constructor and a (non-deleted) copy constructor.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/performance/explicit-move-constructor.html
+class ExplicitMoveConstructorCheck : public ClangTidyCheck {
+public:
+  ExplicitMoveConstructorCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+  bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+    return LangOpts.CPlusPlus;
+  }
+};
+
+} // namespace clang::tidy::performance
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_EXPLICITMOVECONSTRUCTORCHECK_H
diff --git a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
index 9e0fa6f88b36a0..5c222d92dc98f3 100644
--- a/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ b/clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -11,6 +11,7 @@
 #include "../ClangTidyModuleRegistry.h"
 #include "AvoidEndlCheck.h"
 #include "EnumSizeCheck.h"
+#include "ExplicitMoveConstructorCheck.h"
 #include "FasterStringFindCheck.h"
 #include "ForRangeCopyCheck.h"
 #include "ImplicitConversionInLoopCheck.h"
@@ -37,6 +38,8 @@ class PerformanceModule : public ClangTidyModule {
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
     CheckFactories.registerCheck<AvoidEndlCheck>("performance-avoid-endl");
     CheckFactories.registerCheck<EnumSizeCheck>("performance-enum-size");
+    CheckFactories.registerCheck<ExplicitMoveConstructorCheck>(
+        "performance-explicit-move-constructor");
     CheckFactories.registerCheck<FasterStringFindCheck>(
         "performance-faster-string-find");
     CheckFactories.registerCheck<ForRangeCopyCheck>(
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 35cb3e387e4e64..5c3814a8c47000 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -163,6 +163,12 @@ New checks
   Replace comparisons between signed and unsigned integers with their safe
   C++20 ``std::cmp_*`` alternative, if available.
 
+- New :doc:`performance-explicit-move-constructor
+  <clang-tidy/checks/performance/explicit-move-constructor>` check.
+
+  Warns when a class defines an explicit move constructor, which may cause
+  the copy constructor to get called instead.
+
 - New :doc:`portability-template-virtual-member-function
   <clang-tidy/checks/portability/template-virtual-member-function>` check.
 
diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst
index e8f9b4e829634b..43b188d4f347b1 100644
--- a/clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -330,6 +330,7 @@ Clang-Tidy Checks
    :doc:`openmp-use-default-none <openmp/use-default-none>`,
    :doc:`performance-avoid-endl <performance/avoid-endl>`, "Yes"
    :doc:`performance-enum-size <performance/enum-size>`,
+   :doc:`performance-explicit-move-constructor <performance/explicit-move-constructor>`, "Yes"
    :doc:`performance-faster-string-find <performance/faster-string-find>`, "Yes"
    :doc:`performance-for-range-copy <performance/for-range-copy>`, "Yes"
    :doc:`performance-implicit-conversion-in-loop <performance/implicit-conversion-in-loop>`,
diff --git a/clang-tools-extra/docs/clang-tidy/checks/performance/explicit-move-constructor.rst b/clang-tools-extra/docs/clang-tidy/checks/performance/explicit-move-constructor.rst
new file mode 100644
index 00000000000000..97779b1f3edbe7
--- /dev/null
+++ b/clang-tools-extra/docs/clang-tidy/checks/performance/explicit-move-constructor.rst
@@ -0,0 +1,33 @@
+.. title:: clang-tidy - performance-explicit-move-constructor
+
+performance-explicit-move-constructor
+=====================================
+
+Checks for classes that define an explicit move constructor and a copy
+constructor. Moving an instance of such a class will call the copy constructor
+instead.
+
+Example:
+
+.. code-block:: c++
+
+    class Expensive {
+    public:
+        // ...
+        Expensive(const Expensive&) { /* ... */ }
+        explicit Expensive(Expensive&&) { /* ... */ }
+    };
+
+    void process(Expensive);
+
+    int main() {
+        Expensive exp{};
+        process(std::move(exp));
+
+        return 0;
+    }
+
+Here, the call to ``process`` is actually going to copy ``exp`` instead of
+moving it, potentially incurring a performance penalty if copying is expensive.
+No warning will be emitted if the copy constructor is deleted, as any call to
+it would make the program fail to compile.
\ No newline at end of file
diff --git a/clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp b/clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp
new file mode 100644
index 00000000000000..737b7c51b14cf0
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp
@@ -0,0 +1,34 @@
+// RUN: %check_clang_tidy %s performance-explicit-move-constructor %t
+
+class NotReported1 {};
+
+class NotReported2 {
+public:
+  NotReported2(NotReported2&&) = default;
+  NotReported2(const NotReported2&) = default;
+};
+
+class NotReported3 {
+public:
+  explicit NotReported3(NotReported3&&) = default;
+};
+
+class NotReported4 {
+public:
+  explicit NotReported4(NotReported4&&) = default;
+  NotReported4(const NotReported4&) = delete;
+};
+
+class NotReported5 {
+public:
+  explicit NotReported5(NotReported5&&) = delete;
+  NotReported5(const NotReported5&) = default;
+};
+
+class Reported {
+public:
+  explicit Reported(Reported&&) = default;
+  // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: copy constructor may be called instead of move constructor [performance-explicit-move-constructor]
+  // CHECK-FIXES: {{^  }}Reported(Reported&&) = default;{{$}}
+  Reported(const Reported&) = default;
+};

``````````

</details>


https://github.com/llvm/llvm-project/pull/122599