mbid created this revision.
Herald added a subscriber: carlosgalvezp.
Herald added a reviewer: njames93.
Herald added a project: All.
mbid requested review of this revision.
Herald added projects: clang, clang-tools-extra.
Herald added a subscriber: cfe-commits.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D138031
Files:
clang-tools-extra/clang-tidy/performance/CMakeLists.txt
clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.cpp
clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.h
clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/docs/clang-tidy/checks/performance/missing-move-constructor.rst
clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
clang/include/clang/Lex/Token.h
Index: clang/include/clang/Lex/Token.h
===================================================================
--- clang/include/clang/Lex/Token.h
+++ clang/include/clang/Lex/Token.h
@@ -96,9 +96,7 @@
/// "if (Tok.is(tok::l_brace)) {...}".
bool is(tok::TokenKind K) const { return Kind == K; }
bool isNot(tok::TokenKind K) const { return Kind != K; }
- bool isOneOf(tok::TokenKind K1, tok::TokenKind K2) const {
- return is(K1) || is(K2);
- }
+ bool isOneOf() const { return false; }
template <typename... Ts> bool isOneOf(tok::TokenKind K1, Ts... Ks) const {
return is(K1) || isOneOf(Ks...);
}
Index: clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/performance/missing-move-constructor.cpp
@@ -0,0 +1,99 @@
+// RUN: %check_clang_tidy %s performance-missing-move-constructor %t
+
+class OffendingDtor {
+public:
+ ~OffendingDtor() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit destructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingCopyCtor {
+public:
+ OffendingCopyCtor(const OffendingCopyCtor &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingMoveCtor {
+public:
+ OffendingMoveCtor(OffendingMoveCtor &&) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit move constructor prevents generation of implicit move assignment operator [performance-missing-move-constructor]
+
+ // Copy assignment operator is not implicitly defined because of the explicit
+ // move constructor.
+ OffendingMoveCtor &operator=(const OffendingMoveCtor &) = default;
+};
+
+class OffendingCopyAssign {
+public:
+ OffendingCopyAssign &operator=(const OffendingCopyAssign &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy assignment operator prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class OffendingMoveAssign {
+public:
+ // Copy constructor is not implicitly defined because of the explicit move
+ // assignment operator. This means that the warning appears here, because the
+ // copy constructor has higher priority in where we print than the move
+ // assignment operator.
+ OffendingMoveAssign(const OffendingMoveAssign &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor [performance-missing-move-constructor]
+
+ OffendingMoveAssign &operator=(OffendingMoveAssign &&) = default;
+};
+
+
+class InlineFix {
+public:
+ InlineFix(const InlineFix &) = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+ // CHECK-FIXES: InlineFix(InlineFix &&) = default;
+
+ InlineFix& operator=(const InlineFix &) = default;
+ // CHECK-FIXES: InlineFix &operator=(InlineFix &&) = default;
+};
+
+namespace N {
+
+class OutOfLineFix {
+public:
+ OutOfLineFix(const OutOfLineFix &);
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit copy constructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+ // CHECK-FIXES: OutOfLineFix(OutOfLineFix &&);
+
+ OutOfLineFix& operator=(const OutOfLineFix &);
+ // CHECK-FIXES: OutOfLineFix &operator=(OutOfLineFix &&);
+};
+
+}
+
+N::OutOfLineFix::OutOfLineFix(const OutOfLineFix &) = default;
+// CHECK-FIXES: N::OutOfLineFix::OutOfLineFix(OutOfLineFix &&) = default;
+
+N::OutOfLineFix &N::OutOfLineFix::operator=(const OutOfLineFix &) = default;
+// CHECK-FIXES: N::OutOfLineFix &N::OutOfLineFix::operator=(OutOfLineFix &&) = default;
+
+class InlineFixWithoutConstructor {
+ // CHECK-FIXES: InlineFixWithoutConstructor(InlineFixWithoutConstructor &&) = default;
+ // CHECK-FIXES: InlineFixWithoutConstructor &operator=(InlineFixWithoutConstructor &&) = default;
+ ~InlineFixWithoutConstructor() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit destructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class InlineFixWithConstructorWithoutCopy {
+public:
+ InlineFixWithConstructorWithoutCopy() {}
+ // CHECK-FIXES: InlineFixWithConstructorWithoutCopy(InlineFixWithConstructorWithoutCopy &&) = default;
+ // CHECK-FIXES: InlineFixWithConstructorWithoutCopy &operator=(InlineFixWithConstructorWithoutCopy &&) = default;
+
+ ~InlineFixWithConstructorWithoutCopy() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit destructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
+
+class InlineFixWithDefaultedConstructorWithoutCopy {
+public:
+ InlineFixWithDefaultedConstructorWithoutCopy() = default;
+ // CHECK-FIXES: InlineFixWithDefaultedConstructorWithoutCopy(InlineFixWithDefaultedConstructorWithoutCopy &&) = default;
+ // CHECK-FIXES: InlineFixWithDefaultedConstructorWithoutCopy &operator=(InlineFixWithDefaultedConstructorWithoutCopy &&) = default;
+
+ ~InlineFixWithDefaultedConstructorWithoutCopy() = default;
+ // CHECK-MESSAGES: :[[@LINE-1]]:3: warning: explicit destructor prevents generation of implicit move constructor and move assignment operator [performance-missing-move-constructor]
+};
Index: clang-tools-extra/docs/clang-tidy/checks/performance/missing-move-constructor.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/performance/missing-move-constructor.rst
@@ -0,0 +1,25 @@
+.. title:: clang-tidy - performance-missing-move-constructor
+
+performance-missing-move-constructor
+====================================
+
+Warns when a class has a copy constructor but not move constructor.
+
+Classes that have one of the following member methods do not have `implicit move constructors <https://en.cppreference.com/w/cpp/language/move_constructor>`:
+- A copy constructor.
+- An assignment operator.
+- A destructor.
+
+This check warns about classes that are copyable but do not have an implicit
+move constructor because one of the methods above is explicitly declared.
+Similar rules apply to implicit move assignment operators, and the check warns
+also if those are not generated.
+
+.. code-block:: c++
+ class C {
+ // This explicit copy constructor prevents generation of the implicit
+ // move constructor.
+ C(const C &that) {
+ //...
+ }
+ };
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -310,6 +310,7 @@
`performance-inefficient-algorithm <performance/inefficient-algorithm.html>`_, "Yes"
`performance-inefficient-string-concatenation <performance/inefficient-string-concatenation.html>`_,
`performance-inefficient-vector-operation <performance/inefficient-vector-operation.html>`_, "Yes"
+ `performance-missing-move-constructor <performance/missing-move-constructor.html>`_, "Yes"
`performance-move-const-arg <performance/move-const-arg.html>`_, "Yes"
`performance-move-constructor-init <performance/move-constructor-init.html>`_,
`performance-no-automatic-move <performance/no-automatic-move.html>`_,
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -115,6 +115,12 @@
Warns when using ``do-while`` loops.
+- New :doc:`performance-missing-move-constructor
+ <clang-tidy/checks/performance/missing-move-constructor>` check.
+
+ Warns when a class declares a copy constructor but no move constructor, and
+ similarly for assignment operators.
+
New check aliases
^^^^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
+++ clang-tools-extra/clang-tidy/performance/PerformanceTidyModule.cpp
@@ -15,6 +15,7 @@
#include "InefficientAlgorithmCheck.h"
#include "InefficientStringConcatenationCheck.h"
#include "InefficientVectorOperationCheck.h"
+#include "MissingMoveConstructorCheck.h"
#include "MoveConstArgCheck.h"
#include "MoveConstructorInitCheck.h"
#include "NoAutomaticMoveCheck.h"
@@ -44,6 +45,8 @@
"performance-inefficient-string-concatenation");
CheckFactories.registerCheck<InefficientVectorOperationCheck>(
"performance-inefficient-vector-operation");
+ CheckFactories.registerCheck<MissingMoveConstructorCheck>(
+ "performance-missing-move-constructor");
CheckFactories.registerCheck<MoveConstArgCheck>(
"performance-move-const-arg");
CheckFactories.registerCheck<MoveConstructorInitCheck>(
Index: clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.h
@@ -0,0 +1,35 @@
+//===--- MissingMoveConstructorCheck.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_MISSINGMOVECONSTRUCTORCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_MISSINGMOVECONSTRUCTORCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+class MissingMoveConstructorCheck : public ClangTidyCheck {
+public:
+ MissingMoveConstructorCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+
+private:
+ void checkConstructor(ASTContext &Context,
+ const CXXConstructorDecl &CopyCtor);
+ void checkAssignment(ASTContext &Context, const CXXMethodDecl &CopyAssign);
+};
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_MISSINGMOVECONSTRUCTORCHECK_H
Index: clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/performance/MissingMoveConstructorCheck.cpp
@@ -0,0 +1,598 @@
+//===--- MissingMoveConstructorCheck.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 "MissingMoveConstructorCheck.h"
+#include "../utils/LexerUtils.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include <algorithm>
+#include <optional>
+
+using namespace clang::ast_matchers;
+using namespace clang::tidy::utils::lexer;
+
+namespace clang {
+namespace tidy {
+namespace performance {
+
+namespace {
+
+/// A structure that contains explicit declarations preventing implicit
+/// generation of move contsructors or move assignment operators.
+struct ImplicitMovePreventingDecls {
+ const CXXDestructorDecl *Destructor = nullptr;
+
+ const CXXConstructorDecl *CopyConstructor = nullptr;
+ const CXXConstructorDecl *MoveConstructor = nullptr;
+
+ const CXXMethodDecl *CopyAssignment = nullptr;
+ const CXXMethodDecl *MoveAssignment = nullptr;
+};
+
+/// Returns true iff `Decls` contains at least one field that is not nullptr.
+bool isSome(const ImplicitMovePreventingDecls &Decls) {
+ return Decls.Destructor || Decls.CopyConstructor || Decls.MoveConstructor ||
+ Decls.CopyAssignment || Decls.MoveAssignment;
+}
+
+/// Finds all implicit move preventing declarations in the given `Record`.
+///
+/// If there are no such declarations, all fields in the result structure are
+/// null.
+ImplicitMovePreventingDecls
+findImplicitMovePreventingDecls(const CXXRecordDecl &Record) {
+ ImplicitMovePreventingDecls Result;
+
+ // Check for a user-declared destructor.
+ const CXXDestructorDecl *const Dtor = Record.getDestructor();
+ if (Dtor && !Dtor->isImplicit()) {
+ Result.Destructor = Dtor;
+ }
+
+ // Check for move or copy constructors. If there are several of one of the
+ // two types, we select the last ones in declaration order.
+ for (const CXXConstructorDecl *Ctor : Record.ctors()) {
+ if (Ctor->isImplicit()) {
+ continue;
+ }
+
+ if (Ctor->isCopyConstructor()) {
+ Result.CopyConstructor = Ctor;
+ }
+
+ if (Ctor->isMoveConstructor()) {
+ Result.MoveConstructor = Ctor;
+ }
+ }
+
+ // Check for move or copy constructors. If there are several of one of the
+ // two types, we select the last ones in declaration order.
+ for (const CXXMethodDecl *Method : Record.methods()) {
+ if (Method->isImplicit()) {
+ continue;
+ }
+
+ if (Method->isCopyAssignmentOperator()) {
+ Result.CopyAssignment = Method;
+ }
+
+ if (Method->isMoveAssignmentOperator()) {
+ Result.MoveAssignment = Method;
+ }
+ }
+
+ return Result;
+}
+
+/// A class misses a move constructor if it has a copy constructor but no move
+/// constructor.
+bool missesMoveConstructor(const CXXRecordDecl &Record) {
+ const auto CopyCtorIt = std::find_if(
+ Record.ctor_begin(), Record.ctor_end(),
+ [](CXXConstructorDecl *Ctor) { return Ctor->isCopyConstructor(); });
+ const bool HasCopyCtor =
+ CopyCtorIt != Record.ctor_end() || Record.needsImplicitCopyConstructor();
+
+ const auto MoveCtorIt = std::find_if(
+ Record.ctor_begin(), Record.ctor_end(),
+ [](CXXConstructorDecl *Ctor) { return Ctor->isMoveConstructor(); });
+ const bool HasMoveCtor = MoveCtorIt != Record.ctor_end();
+
+ return HasCopyCtor && !HasMoveCtor;
+}
+
+/// A class misses a move assignment operator if it has a copy assignment
+/// operator but no move assignment operator.
+bool missesMoveAssignmentOperator(const CXXRecordDecl &Record) {
+ const auto CopyAssignIt = std::find_if(
+ Record.method_begin(), Record.method_end(),
+ [](CXXMethodDecl *Method) { return Method->isCopyAssignmentOperator(); });
+ const bool HasCopyAssign = CopyAssignIt != Record.method_end() ||
+ Record.needsImplicitCopyAssignment();
+
+ const auto MoveAssignIt = std::find_if(
+ Record.method_begin(), Record.method_end(),
+ [](CXXMethodDecl *Method) { return Method->isMoveAssignmentOperator(); });
+ const bool HasMoveAssign = MoveAssignIt != Record.method_end();
+
+ return HasCopyAssign && !HasMoveAssign;
+}
+
+std::pair<SourceLocation, std::string>
+diagnosticMessage(const ImplicitMovePreventingDecls &OffendingDecls,
+ const bool MissesMoveConstructor,
+ const bool MissesMoveAssignmentOperator) {
+ SourceLocation Loc;
+ std::string MessageString;
+ llvm::raw_string_ostream Message(MessageString);
+
+ Message << "explicit";
+
+ if (OffendingDecls.Destructor != nullptr) {
+ Message << " destructor";
+ Loc = OffendingDecls.Destructor->getBeginLoc();
+ } else if (OffendingDecls.CopyConstructor != nullptr) {
+ Message << " copy constructor";
+ Loc = OffendingDecls.CopyConstructor->getBeginLoc();
+ } else if (OffendingDecls.MoveConstructor != nullptr) {
+ Message << " move constructor";
+ Loc = OffendingDecls.MoveConstructor->getBeginLoc();
+ } else if (OffendingDecls.CopyAssignment != nullptr) {
+ Message << " copy assignment operator";
+ Loc = OffendingDecls.CopyAssignment->getBeginLoc();
+ } else if (OffendingDecls.MoveAssignment != nullptr) {
+ Message << " move assignment operator";
+ Loc = OffendingDecls.MoveAssignment->getBeginLoc();
+ } else {
+ llvm_unreachable("no offending declaration");
+ }
+
+ Message << " prevents generation of implicit";
+ if (MissesMoveConstructor && MissesMoveAssignmentOperator) {
+ Message << " move constructor and move assignment operator";
+ } else if (!MissesMoveConstructor && MissesMoveAssignmentOperator) {
+ Message << " move assignment operator";
+ } else if (MissesMoveConstructor && !MissesMoveAssignmentOperator) {
+ Message << " move constructor";
+ } else {
+ llvm_unreachable("nothing missing");
+ }
+
+ return {Loc, std::move(MessageString)};
+}
+
+enum class NewlinePosition {
+ before,
+ after,
+};
+
+/// An ostream that is meant to print a single line into an std::string. Will
+/// insert a newline character before or after the line depending on a
+/// `NewlinePosition`.
+class LineRawStringOstream {
+public:
+ LineRawStringOstream(NewlinePosition NewlinePos) : NewlinePos(NewlinePos) {
+ if (NewlinePos == NewlinePosition::before) {
+ *this << "\n";
+ }
+ }
+
+ // No copy/move: We have fields referencing each other.
+ LineRawStringOstream(const LineRawStringOstream &) = delete;
+ LineRawStringOstream &operator=(const LineRawStringOstream &) = delete;
+
+ template <class T> LineRawStringOstream &operator<<(T &&Arg) {
+ Stream << std::forward<T>(Arg);
+ return *this;
+ }
+
+ /// Retrieve the line as a string.
+ std::string str() && {
+ if (NewlinePos == NewlinePosition::after) {
+ *this << "\n";
+ }
+ return std::move(String);
+ }
+
+private:
+ NewlinePosition NewlinePos;
+ std::string String;
+ llvm::raw_string_ostream Stream{String};
+};
+
+/// Information about where a method declaration-definition pair should be
+/// emitted.
+struct MethodInsertInfo {
+ // If DeclarationLocation and DefinitionLocation agree, then the method
+ // should be defined inline. Otherwise, separate declaration and definition
+ // should be emitted.
+ SourceLocation DeclarationLocation;
+ SourceLocation DefinitionLocation;
+
+ // Fields describing whether a newline should be added before the emitted
+ // code or after.
+ NewlinePosition NewlinePos;
+
+ // Unqualified name of the record/class this method belongs to.
+ std::string RecordName;
+ // A qualified version of RecordName that can be use to refer to the type at
+ // DefinitionLocation.
+ std::string QualifiedRecordName;
+};
+
+void GenerateMoveConstructor(DiagnosticBuilder &Diagnostic,
+ const MethodInsertInfo &InsertInfo) {
+ if (InsertInfo.DeclarationLocation == InsertInfo.DefinitionLocation) {
+ LineRawStringOstream MoveCtor{InsertInfo.NewlinePos};
+ MoveCtor << InsertInfo.RecordName << "(" << InsertInfo.RecordName
+ << " &&) = default;";
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DeclarationLocation,
+ std::move(MoveCtor).str());
+ } else {
+ LineRawStringOstream MoveCtorDecl(InsertInfo.NewlinePos);
+ MoveCtorDecl << InsertInfo.RecordName << "(" << InsertInfo.RecordName
+ << " &&);";
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DeclarationLocation,
+ std::move(MoveCtorDecl).str());
+
+ LineRawStringOstream MoveCtorDef(InsertInfo.NewlinePos);
+ MoveCtorDef << InsertInfo.QualifiedRecordName
+ << "::" << InsertInfo.RecordName << "(" << InsertInfo.RecordName
+ << " &&) = default;";
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DefinitionLocation,
+ std::move(MoveCtorDef).str());
+ }
+}
+
+void GenerateMoveAssignment(DiagnosticBuilder &Diagnostic,
+ const MethodInsertInfo &InsertInfo) {
+ if (InsertInfo.DeclarationLocation == InsertInfo.DefinitionLocation) {
+ LineRawStringOstream MoveAssign{InsertInfo.NewlinePos};
+ MoveAssign << InsertInfo.RecordName << " &operator=("
+ << InsertInfo.RecordName << " &&) = default;";
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DeclarationLocation,
+ std::move(MoveAssign).str());
+ } else {
+ LineRawStringOstream MoveAssignDecl{InsertInfo.NewlinePos};
+ MoveAssignDecl << InsertInfo.RecordName << " &operator=("
+ << InsertInfo.RecordName << " &&);";
+
+ LineRawStringOstream MoveAssignDef{InsertInfo.NewlinePos};
+ MoveAssignDef << InsertInfo.QualifiedRecordName << " &"
+ << InsertInfo.QualifiedRecordName << "::operator=("
+ << InsertInfo.RecordName << " &&) = default;";
+
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DeclarationLocation,
+ std::move(MoveAssignDecl).str());
+ Diagnostic << FixItHint::CreateInsertion(InsertInfo.DefinitionLocation,
+ std::move(MoveAssignDef).str());
+ }
+}
+
+/// Retrieve the way an out-of-line method declaration refers to the type it
+/// belongs to.
+std::optional<std::string>
+QualifiedOutOfLineRecordName(const LangOptions &LangOpts,
+ const CXXMethodDecl &MethodDecl) {
+ std::string ResultString;
+ {
+ llvm::raw_string_ostream Result{ResultString};
+ MethodDecl.getQualifier()->print(Result, PrintingPolicy(LangOpts));
+ }
+
+ // ResultString should contain the suffix "::". We need to remove this
+ // because we want the string refering to the type itself.
+ if (StringRef(ResultString).substr(ResultString.size() - 2) != "::") {
+ return std::nullopt;
+ }
+ ResultString.resize(ResultString.size() - 2);
+
+ return ResultString;
+}
+
+/// Retrieve info for insertion after a specified method.
+std::optional<MethodInsertInfo>
+MethodInsertInfoAfterMethod(const CXXMethodDecl &Method, SourceManager &SrcMgr,
+ const LangOptions &LangOpts) {
+ const CXXRecordDecl *Record = Method.getParent();
+ if (!Record) {
+ return std::nullopt;
+ }
+
+ const CXXMethodDecl *Def = cast<CXXMethodDecl>(Method.getDefinition());
+ const CXXMethodDecl *Decl = Method.getCanonicalDecl();
+ if (!Def || !Decl) {
+ return std::nullopt;
+ }
+
+ SourceLocation DefinitionLocation;
+ if (const Stmt *Body = Def->getBody()) {
+ DefinitionLocation = Body->getEndLoc().getLocWithOffset(1);
+ } else if (Def->isDefaulted() || Def->isDeleted()) {
+ DefinitionLocation =
+ findNextAnyTokenKind(Def->getEndLoc(), SrcMgr, LangOpts, tok::semi)
+ .getLocWithOffset(1);
+ } else {
+ return std::nullopt;
+ }
+
+ const SourceLocation DeclarationLocation =
+ Def == Decl
+ ? DefinitionLocation
+ : findNextAnyTokenKind(Decl->getEndLoc(), SrcMgr, LangOpts, tok::semi)
+ .getLocWithOffset(1);
+
+ std::string QualifiedRecordName;
+ if (Def == Decl) {
+ QualifiedRecordName = Record->getName().str();
+ } else {
+ std::optional<std::string> QualName =
+ QualifiedOutOfLineRecordName(LangOpts, *Def);
+ if (!QualName.has_value()) {
+ return std::nullopt;
+ }
+ QualifiedRecordName = std::move(*QualName);
+ }
+
+ return MethodInsertInfo{
+ DeclarationLocation,
+ DefinitionLocation,
+ NewlinePosition::before,
+ Record->getName().str(),
+ std::move(QualifiedRecordName),
+ };
+}
+
+/// Retrieve info for insertion before a specified method.
+std::optional<MethodInsertInfo>
+MethodInsertInfoBeforeMethod(const CXXMethodDecl &Method, SourceManager &SrcMgr,
+ const LangOptions &LangOpts) {
+ const CXXRecordDecl *Record = Method.getParent();
+ if (!Record) {
+ return std::nullopt;
+ }
+
+ const CXXMethodDecl *Def = cast<CXXMethodDecl>(Method.getDefinition());
+ const CXXMethodDecl *Decl = Method.getCanonicalDecl();
+ if (!Def || !Decl) {
+ return std::nullopt;
+ }
+
+ const SourceLocation DefinitionLocation = Def->getBeginLoc();
+ const SourceLocation DeclarationLocation = Decl->getBeginLoc();
+
+ std::string QualifiedRecordName;
+ if (Def == Decl) {
+ QualifiedRecordName = Record->getName().str();
+ } else {
+ std::optional<std::string> QualName =
+ QualifiedOutOfLineRecordName(LangOpts, *Def);
+ if (!QualName.has_value()) {
+ return std::nullopt;
+ }
+ QualifiedRecordName = std::move(*QualName);
+ }
+
+ return MethodInsertInfo{
+ DeclarationLocation,
+ DefinitionLocation,
+ NewlinePosition::after,
+ Record->getName().str(),
+ std::move(QualifiedRecordName),
+ };
+}
+
+const CXXConstructorDecl *FindLastConstructor(const CXXRecordDecl &Record,
+ SourceManager &SrcMgr) {
+ const CXXConstructorDecl *LastCtor = nullptr;
+ for (CXXConstructorDecl *Ctor : Record.ctors()) {
+ if (!LastCtor || SrcMgr.isBeforeInTranslationUnit(LastCtor->getBeginLoc(),
+ Ctor->getBeginLoc())) {
+ LastCtor = Ctor;
+ }
+ }
+ return LastCtor;
+}
+
+const CXXMethodDecl *FindFirstAssignmentOperator(const CXXRecordDecl &Record,
+ SourceManager &SrcMgr) {
+ const CXXMethodDecl *FirstAssign = nullptr;
+ for (CXXMethodDecl *Method : Record.methods()) {
+ if (!Method->isCopyAssignmentOperator() &&
+ !Method->isCopyAssignmentOperator()) {
+ continue;
+ }
+
+ if (!FirstAssign ||
+ SrcMgr.isBeforeInTranslationUnit(Method->getBeginLoc(),
+ FirstAssign->getBeginLoc())) {
+ FirstAssign = Method;
+ }
+ }
+
+ return FirstAssign;
+}
+
+/// Find a general location where a move constructor or move assignment
+/// operator should be defined/declared. This follows the google style guide:
+/// After constructors, before assignment operators, before destructors.
+///
+/// This method should be called in case there is no obvious place to put the
+/// move method (i.e. there is no user-declared copying version).
+std::optional<MethodInsertInfo>
+FindMoveMethodLocation(const LangOptions &LangOpts, SourceManager &SrcMgr,
+ const CXXRecordDecl &Record) {
+ const CXXConstructorDecl *LastCtor = FindLastConstructor(Record, SrcMgr);
+ if (LastCtor) {
+ return MethodInsertInfoAfterMethod(*LastCtor, SrcMgr, LangOpts);
+ }
+
+ const CXXMethodDecl *LastAssign = FindFirstAssignmentOperator(Record, SrcMgr);
+ if (LastCtor) {
+ return MethodInsertInfoBeforeMethod(*LastAssign, SrcMgr, LangOpts);
+ }
+
+ const CXXDestructorDecl *Destructor = Record.getDestructor();
+ if (Destructor) {
+ return MethodInsertInfoBeforeMethod(*Destructor, SrcMgr, LangOpts);
+ }
+
+ return std::nullopt;
+}
+
+/// Try to emit FixIt hints that fix a missing move constructor.
+void FixMoveConstructor(DiagnosticBuilder &Diagnostic,
+ const CXXRecordDecl &Record, SourceManager &SrcMgr,
+ const LangOptions &LangOpts) {
+ if (Record.needsImplicitCopyConstructor()) {
+ std::optional<MethodInsertInfo> InsertInfo =
+ FindMoveMethodLocation(LangOpts, SrcMgr, Record);
+ if (InsertInfo.has_value()) {
+ GenerateMoveConstructor(Diagnostic, *InsertInfo);
+ }
+ return;
+ }
+
+ // Find definition and declaration of the copy constructor. The new move
+ // constructor should be defined/declared next to the copy constructor. If we
+ // can't find declaration or definition of the copy constructor, we can't
+ // suggest a fix.
+ const auto CopyCtorIt = std::find_if(
+ Record.ctor_begin(), Record.ctor_end(),
+ [](CXXConstructorDecl *Ctor) { return Ctor->isCopyConstructor(); });
+ if (CopyCtorIt == Record.ctor_end()) {
+ return;
+ }
+ const CXXConstructorDecl *CopyCtor = *CopyCtorIt;
+
+ const FunctionDecl *CopyCtorDef = CopyCtor->getDefinition();
+ if (!CopyCtorDef || !CopyCtorDef->isDefaulted()) {
+ // We only know how to generate default move constructors. If a class needs
+ // a custom copy constructor, then it likely needs a custom move
+ // constructor, so we're not trying to automatically fix this case.
+ return;
+ }
+
+ std::optional<MethodInsertInfo> InsertInfo =
+ MethodInsertInfoAfterMethod(*CopyCtor, SrcMgr, LangOpts);
+ if (InsertInfo) {
+ GenerateMoveConstructor(Diagnostic, *InsertInfo);
+ }
+}
+
+/// Try to emit FixIt hints that fix a missing move assignment operator.
+void FixMoveAssignment(DiagnosticBuilder &Diagnostic,
+ const CXXRecordDecl &Record, SourceManager &SrcMgr,
+ const LangOptions &LangOpts) {
+ // Move assignments are generated analogously to move constructors.
+
+ if (Record.needsImplicitCopyAssignment()) {
+ std::optional<MethodInsertInfo> InsertInfo =
+ FindMoveMethodLocation(LangOpts, SrcMgr, Record);
+ if (InsertInfo.has_value()) {
+ GenerateMoveAssignment(Diagnostic, *InsertInfo);
+ }
+ return;
+ }
+
+ const auto CopyAssignIt = std::find_if(
+ Record.method_begin(), Record.method_end(),
+ [](CXXMethodDecl *Method) { return Method->isCopyAssignmentOperator(); });
+ if (CopyAssignIt == Record.method_end()) {
+ return;
+ }
+ const CXXMethodDecl *CopyAssign = *CopyAssignIt;
+
+ const FunctionDecl *CopyAssignDef = CopyAssign->getDefinition();
+ if (!CopyAssignDef || !CopyAssignDef->isDefaulted()) {
+ return;
+ }
+
+ std::optional<MethodInsertInfo> InsertInfo =
+ MethodInsertInfoAfterMethod(*CopyAssign, SrcMgr, LangOpts);
+ if (InsertInfo) {
+ GenerateMoveAssignment(Diagnostic, *InsertInfo);
+ }
+}
+
+} // namespace
+
+void MissingMoveConstructorCheck::registerMatchers(MatchFinder *Finder) {
+ Finder->addMatcher(
+ traverse(TK_IgnoreUnlessSpelledInSource,
+ cxxConstructorDecl(isCopyConstructor(), isDefinition())
+ .bind("CopyCtor")),
+ this);
+
+ Finder->addMatcher(
+ traverse(TK_IgnoreUnlessSpelledInSource,
+ cxxMethodDecl(isCopyAssignmentOperator(), isDefinition())
+ .bind("CopyAssign")),
+ this);
+
+ Finder->addMatcher(traverse(TK_IgnoreUnlessSpelledInSource,
+ cxxDestructorDecl(isDefinition()).bind("Dtor")),
+ this);
+}
+
+void MissingMoveConstructorCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ ASTContext &Context = *Result.Context;
+
+ const CXXRecordDecl *Record = nullptr;
+
+ if (auto *CopyCtor = Result.Nodes.getNodeAs<CXXConstructorDecl>("CopyCtor")) {
+ Record = CopyCtor->getParent();
+ } else if (auto *CopyAssign =
+ Result.Nodes.getNodeAs<CXXMethodDecl>("CopyAssign")) {
+ Record = CopyAssign->getParent();
+ } else if (auto *Dtor = Result.Nodes.getNodeAs<CXXDestructorDecl>("Dtor")) {
+ Record = Dtor->getParent();
+ } else {
+ llvm_unreachable("Impossible match");
+ }
+ assert(Record);
+
+ const ImplicitMovePreventingDecls OffendingDecls =
+ findImplicitMovePreventingDecls(*Record);
+ if (!isSome(OffendingDecls)) {
+ return;
+ }
+ // At this point, we know that the implicit move constructor and the implicit
+ // move assignment operator are not generated for `Record`.
+
+ const bool MissesMoveCtor = missesMoveConstructor(*Record);
+ const bool MissesMoveAssign = missesMoveAssignmentOperator(*Record);
+
+ if (!MissesMoveCtor && !MissesMoveAssign) {
+ return;
+ }
+
+ auto [Loc, Message] = diagnosticMessage(
+ OffendingDecls, /* MissesMoveConstructor = */ MissesMoveCtor,
+ /* MissesMoveAssignmentOperator = */ MissesMoveAssign);
+
+ auto Diag = diag(Loc, std::move(Message));
+
+ const LangOptions &LangOpts = Context.getLangOpts();
+ SourceManager &SrcMgr = Context.getSourceManager();
+
+ if (MissesMoveCtor) {
+ FixMoveConstructor(Diag, *Record, SrcMgr, LangOpts);
+ }
+ if (MissesMoveAssign) {
+ FixMoveAssignment(Diag, *Record, SrcMgr, LangOpts);
+ }
+}
+
+} // namespace performance
+} // namespace tidy
+} // namespace clang
Index: clang-tools-extra/clang-tidy/performance/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/performance/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/performance/CMakeLists.txt
@@ -10,6 +10,7 @@
InefficientAlgorithmCheck.cpp
InefficientStringConcatenationCheck.cpp
InefficientVectorOperationCheck.cpp
+ MissingMoveConstructorCheck.cpp
MoveConstArgCheck.cpp
MoveConstructorInitCheck.cpp
NoAutomaticMoveCheck.cpp
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
