[clang-tools-extra] [clang-tidy] Add `performance-explicit-move-constructor` check (PR #122599)
via cfe-commits
cfe-commits at lists.llvm.org
Sat Jan 11 08:38:18 PST 2025
https://github.com/dodicidodici created https://github.com/llvm/llvm-project/pull/122599
This patch adds a check that reports classes that define an explicit move constructor and a copy constructor.
Closes #121707
>From 2f1627ee62c041d98cbc51b08aebf2dd2dbe5455 Mon Sep 17 00:00:00 2001
From: dodicidodici <dodicilosanno at gmail.com>
Date: Fri, 10 Jan 2025 19:05:09 +0100
Subject: [PATCH 1/5] add `performance-explicit-move-constructor` check
---
.../ExplicitMoveConstructorCheck.cpp | 72 +++++++++++++++++++
.../ExplicitMoveConstructorCheck.h | 33 +++++++++
2 files changed, 105 insertions(+)
create mode 100644 clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
create mode 100644 clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
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..631ed363566c65
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
@@ -0,0 +1,72 @@
+//===--- 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())
+ .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 explicit 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
>From 2f0b72273c4d471540b7f4b04da9073ca670fa0f Mon Sep 17 00:00:00 2001
From: dodicidodici <dodicilosanno at gmail.com>
Date: Fri, 10 Jan 2025 19:26:02 +0100
Subject: [PATCH 2/5] mention in ReleaseNotes.rst
---
clang-tools-extra/docs/ReleaseNotes.rst | 6 ++++++
1 file changed, 6 insertions(+)
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.
>From 56acf544b72040d01f48e3af566561f0b3a54582 Mon Sep 17 00:00:00 2001
From: dodicidodici <dodicilosanno at gmail.com>
Date: Fri, 10 Jan 2025 19:28:46 +0100
Subject: [PATCH 3/5] actually register the check
---
clang-tools-extra/clang-tidy/performance/CMakeLists.txt | 1 +
.../clang-tidy/performance/PerformanceTidyModule.cpp | 3 +++
2 files changed, 4 insertions(+)
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/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>(
>From d3a09b5b1c528c6a361af822c915bcb23313bd5d Mon Sep 17 00:00:00 2001
From: dodicidodici <dodicilosanno at gmail.com>
Date: Sat, 11 Jan 2025 15:41:25 +0100
Subject: [PATCH 4/5] add documentation for
`performance-explicit-move-constructor`
---
.../docs/clang-tidy/checks/list.rst | 1 +
.../performance/explicit-move-constructor.rst | 33 +++++++++++++++++++
2 files changed, 34 insertions(+)
create mode 100644 clang-tools-extra/docs/clang-tidy/checks/performance/explicit-move-constructor.rst
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
>From 9280db3d56d7633bc36a64e118425c13ebafffd5 Mon Sep 17 00:00:00 2001
From: dodicidodici <dodicilosanno at gmail.com>
Date: Sat, 11 Jan 2025 17:28:33 +0100
Subject: [PATCH 5/5] add test
---
.../ExplicitMoveConstructorCheck.cpp | 2 +-
.../performance/explicit-move-constructor.cpp | 34 +++++++++++++++++++
2 files changed, 35 insertions(+), 1 deletion(-)
create mode 100644 clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp
diff --git a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
index 631ed363566c65..1a5e92db102150 100644
--- a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
+++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp
@@ -58,7 +58,7 @@ void ExplicitMoveConstructorCheck::check(
auto Diag = diag(
MoveCtor->getLocation(),
- "copy constructor may be called instead of explicit move constructor");
+ "copy constructor may be called instead of move constructor");
SourceRange ExplicitTokenRange =
findExplicitToken(MoveCtor, *Result.SourceManager, getLangOpts());
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..b40d072fc99c45
--- /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;
+};
More information about the cfe-commits
mailing list