llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) <details> <summary>Changes</summary> Fixed false positives when class passed by const-reference had a private move constructor, which could not be used for a fix-it. --- Full diff: https://github.com/llvm/llvm-project/pull/141304.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp (+44-11) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4) - (modified) clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp (+59) ``````````diff diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp index 35f90fb8da15b..343cc966b7e09 100644 --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp @@ -20,8 +20,21 @@ using namespace llvm; namespace clang::tidy::modernize { +static bool isFirstFriendOfSecond(const CXXRecordDecl *Friend, + const CXXRecordDecl *Class) { + return llvm::any_of( + Class->friends(), [Friend](FriendDecl *FriendDecl) -> bool { + if (TypeSourceInfo *FriendTypeSource = FriendDecl->getFriendType()) { + const QualType FriendType = FriendTypeSource->getType(); + return FriendType->getAsCXXRecordDecl() == Friend; + } + return false; + }); +} + namespace { -/// Matches move-constructible classes. +/// Matches move-constructible classes whose constructor can be called inside +/// a CXXRecordDecl with a bound ID. /// /// Given /// \code @@ -32,15 +45,33 @@ namespace { /// Bar(Bar &&) = deleted; /// int a; /// }; +/// +/// class Buz { +/// Buz(Buz &&); +/// int a; +/// friend class Outer; +/// }; +/// +/// class Outer { +/// }; /// \endcode -/// recordDecl(isMoveConstructible()) -/// matches "Foo". -AST_MATCHER(CXXRecordDecl, isMoveConstructible) { - for (const CXXConstructorDecl *Ctor : Node.ctors()) { - if (Ctor->isMoveConstructor() && !Ctor->isDeleted()) - return true; - } - return false; +/// recordDecl(isMoveConstructibleInBoundCXXRecordDecl("Outer")) +/// matches "Foo", "Buz". +AST_MATCHER_P(CXXRecordDecl, isMoveConstructibleInBoundCXXRecordDecl, StringRef, + RecordDeclID) { + return Builder->removeBindings( + [this, + &Node](const ast_matchers::internal::BoundNodesMap &Nodes) -> bool { + const auto *BoundClass = + Nodes.getNode(this->RecordDeclID).get<CXXRecordDecl>(); + for (const CXXConstructorDecl *Ctor : Node.ctors()) { + if (Ctor->isMoveConstructor() && !Ctor->isDeleted() && + (Ctor->getAccess() == AS_public || + (BoundClass && isFirstFriendOfSecond(BoundClass, &Node)))) + return false; + } + return true; + }); } } // namespace @@ -202,6 +233,7 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) { traverse( TK_AsIs, cxxConstructorDecl( + ofClass(cxxRecordDecl().bind("outer")), forEachConstructorInitializer( cxxCtorInitializer( unless(isBaseInitializer()), @@ -225,8 +257,9 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) { .bind("Param"))))), hasDeclaration(cxxConstructorDecl( isCopyConstructor(), unless(isDeleted()), - hasDeclContext( - cxxRecordDecl(isMoveConstructible()))))))) + hasDeclContext(cxxRecordDecl( + isMoveConstructibleInBoundCXXRecordDecl( + "outer")))))))) .bind("Initializer"))) .bind("Ctor")), this); diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 8032f0d23f494..75f3471070646 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -203,6 +203,10 @@ Changes in existing checks excluding variables with ``thread_local`` storage class specifier from being matched. +- Improved :doc:`modernize-pass-by-value + <clang-tidy/checks/modernize/pass-by-value>` check by fixing false positives + when class passed by const-reference had a private move constructor. + - Improved :doc:`modernize-use-default-member-init <clang-tidy/checks/modernize/use-default-member-init>` check by matching ``constexpr`` and ``static``` values on member initialization and by detecting diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp index be33988607b27..7538862dd83f8 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value.cpp @@ -252,3 +252,62 @@ struct W3 { W3(W1 &&, Movable &&M); Movable M; }; + +struct ProtectedMovable { + ProtectedMovable() = default; + ProtectedMovable(const ProtectedMovable &) {} +protected: + ProtectedMovable(ProtectedMovable &&) {} +}; + +struct PrivateMovable { + PrivateMovable() = default; + PrivateMovable(const PrivateMovable &) {} +private: + PrivateMovable(PrivateMovable &&) {} + + friend struct X5; +}; + +struct InheritedProtectedMovable : ProtectedMovable { + InheritedProtectedMovable() = default; + InheritedProtectedMovable(const InheritedProtectedMovable &) {} + InheritedProtectedMovable(InheritedProtectedMovable &&) {} +}; + +struct InheritedPrivateMovable : PrivateMovable { + InheritedPrivateMovable() = default; + InheritedPrivateMovable(const InheritedPrivateMovable &) {} + InheritedPrivateMovable(InheritedPrivateMovable &&) {} +}; + +struct X1 { + X1(const ProtectedMovable &M) : M(M) {} + ProtectedMovable M; +}; + +struct X2 { + X2(const PrivateMovable &M) : M(M) {} + PrivateMovable M; +}; + +struct X3 { + X3(const InheritedProtectedMovable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move + // CHECK-FIXES: X3(InheritedProtectedMovable M) : M(std::move(M)) {} + InheritedProtectedMovable M; +}; + +struct X4 { + X4(const InheritedPrivateMovable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move + // CHECK-FIXES: X4(InheritedPrivateMovable M) : M(std::move(M)) {} + InheritedPrivateMovable M; +}; + +struct X5 { + X5(const PrivateMovable &M) : M(M) {} + // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move + // CHECK-FIXES: X5(PrivateMovable M) : M(std::move(M)) {} + PrivateMovable M; +}; `````````` </details> https://github.com/llvm/llvm-project/pull/141304 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
