[clang-tidy] Add performance-explicit-move-constructor check
#122599
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thank you for submitting a Pull Request (PR) to the LLVM Project! This PR will be automatically labeled and the relevant teams will be notified. If you wish to, you can add reviewers by using the "Reviewers" section on this page. If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers. If you have further questions, they may be answered by the LLVM GitHub User Guide. You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums. |
don't warn on deleted move & fix compilation error on linux
|
@llvm/pr-subscribers-clang-tidy Author: dodicidodici (dodicidodici) ChangesThis patch adds a check that reports classes that define an explicit move constructor and a copy constructor. Full diff: https://github.com/llvm/llvm-project/pull/122599.diff 8 Files Affected:
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;
+};
|
|
@llvm/pr-subscribers-clang-tools-extra Author: dodicidodici (dodicidodici) ChangesThis patch adds a check that reports classes that define an explicit move constructor and a copy constructor. Full diff: https://github.com/llvm/llvm-project/pull/122599.diff 8 Files Affected:
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;
+};
|
|
This check sounds a bit strange. Is this issue common in real-world projects, do we have some data? It's the first time I've heard of it. Why would people deviate from the standard signature for move constructors? We could perhaps consider making this check a bit more general and enforce that all 5 special member functions have the correct signature (including ref-qualification and |
You can test this locally with the following command:git-clang-format --diff ac604b2fa6ff0344a555954069721c0db7b874f9 828461273ea7ac1d38a4d3fe1ff8b810f80472e9 --extensions h,cpp -- clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h clang-tools-extra/test/clang-tidy/checkers/performance/explicit-move-constructor.cpp clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
index 9ae071a20a..a41304aca0 100644
--- a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
+++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.h
@@ -13,7 +13,8 @@
namespace clang::tidy::performance {
-/// Find classes that define an explicit move constructor and a (non-deleted) copy constructor.
+/// 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
|
|
I prefer to make it in bugprone and warn for all kinds of incorrect signatures for these special functions. what do you think.@5chmidti @carlosgalvezp @PiotrZSL |
It's anecdotal, but I've encountered it in a few places in a private codebase. I don't think it's a really common issue, but it'd be nice to have a lint warning for this.
There's |
| const SourceManager &Source, | ||
| const LangOptions &LangOpts) { | ||
| SourceLocation CurrentLoc = Ctor->getBeginLoc(); | ||
| SourceLocation EndLoc = Ctor->getEndLoc(); |
There was a problem hiding this comment.
Suggested change
| SourceLocation EndLoc = Ctor->getEndLoc(); | |
| const SourceLocation EndLoc = Ctor->getEndLoc(); |
| 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; |
| performance-explicit-move-constructor | ||
| ===================================== | ||
|
|
||
| Checks for classes that define an explicit move constructor and a copy |
There was a problem hiding this comment.
Please synchronize first statement with Release Notes.
It may happen. Some C++ newer will blindly follow some coding guideline and add explicit for constructor with only one parameter. And it is hard to detect to be honest since small performance issues are always hard to detect. But I wonder whether is it better to detect explicit for all copy and move constructor instead of only check move constructor is explicit but copy is not. |
PiotrZSL
reviewed
Jan 13, 2025
| void ExplicitMoveConstructorCheck::registerMatchers(MatchFinder *Finder) { | ||
| Finder->addMatcher( | ||
| cxxRecordDecl( | ||
| has(cxxConstructorDecl(isMoveConstructor(), isExplicit(), |
There was a problem hiding this comment.
Maybe exclude implicit code, in such case it would be faster.
Also consider excluding system code.
There was a problem hiding this comment.
That would be with isWritten right?
There was a problem hiding this comment.
use TK_IgnoreUnlessSpelledInSource (best but may impact template instances) or isUserProvided, for system headers is isExpansionInSystemHeader
| // 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; | ||
| }; |
There was a problem hiding this comment.
test with inherited constructors and with template classes
|
Making this more generic sounds good to me. There is also |
So warning for signatures like |
| auto Diag = | ||
| diag(MoveCtor->getLocation(), | ||
| "copy constructor may be called instead of move constructor"); | ||
| SourceRange ExplicitTokenRange = |
There was a problem hiding this comment.
Suggested change
| SourceRange ExplicitTokenRange = | |
| const SourceRange ExplicitTokenRange = |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This patch adds a check that reports classes that define an explicit move constructor and a copy constructor.
Closes #121707