https://github.com/dodicidodici updated https://github.com/llvm/llvm-project/pull/122599
>From 2f1627ee62c041d98cbc51b08aebf2dd2dbe5455 Mon Sep 17 00:00:00 2001 From: dodicidodici <dodicilosa...@gmail.com> Date: Fri, 10 Jan 2025 19:05:09 +0100 Subject: [PATCH 1/6] 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 <dodicilosa...@gmail.com> Date: Fri, 10 Jan 2025 19:26:02 +0100 Subject: [PATCH 2/6] 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 <dodicilosa...@gmail.com> Date: Fri, 10 Jan 2025 19:28:46 +0100 Subject: [PATCH 3/6] 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 <dodicilosa...@gmail.com> Date: Sat, 11 Jan 2025 15:41:25 +0100 Subject: [PATCH 4/6] 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 <dodicilosa...@gmail.com> Date: Sat, 11 Jan 2025 17:28:33 +0100 Subject: [PATCH 5/6] 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; +}; >From 828461273ea7ac1d38a4d3fe1ff8b810f80472e9 Mon Sep 17 00:00:00 2001 From: dodicidodici <dodicilosa...@gmail.com> Date: Sat, 11 Jan 2025 17:56:43 +0100 Subject: [PATCH 6/6] (hopefully) fix ci don't warn on deleted move & fix compilation error on linux --- .../performance/ExplicitMoveConstructorCheck.cpp | 11 ++++++----- .../performance/explicit-move-constructor.cpp | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp index 1a5e92db102150..94f9cebae69506 100644 --- a/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp +++ b/clang-tools-extra/clang-tidy/performance/ExplicitMoveConstructorCheck.cpp @@ -8,7 +8,7 @@ #include "ExplicitMoveConstructorCheck.h" #include "clang/ASTMatchers/ASTMatchFinder.h" -#include "clang/Lex/lexer.h" +#include "clang/Lex/Lexer.h" using namespace clang::ast_matchers; @@ -39,7 +39,8 @@ static SourceRange findExplicitToken(const CXXConstructorDecl *Ctor, void ExplicitMoveConstructorCheck::registerMatchers(MatchFinder *Finder) { Finder->addMatcher( cxxRecordDecl( - has(cxxConstructorDecl(isMoveConstructor(), isExplicit()) + has(cxxConstructorDecl(isMoveConstructor(), isExplicit(), + unless(isDeleted())) .bind("move-ctor")), has(cxxConstructorDecl(isCopyConstructor(), unless(isDeleted())) .bind("copy-ctor"))), @@ -56,9 +57,9 @@ void ExplicitMoveConstructorCheck::check( if (!MoveCtor || !CopyCtor) return; - auto Diag = diag( - MoveCtor->getLocation(), - "copy constructor may be called instead of move constructor"); + auto Diag = + diag(MoveCtor->getLocation(), + "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 index b40d072fc99c45..737b7c51b14cf0 100644 --- 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 @@ -29,6 +29,6 @@ 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;{{$}} + // CHECK-FIXES: {{^ }}Reported(Reported&&) = default;{{$}} Reported(const Reported&) = default; }; _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
