[clang-tools-extra] [clang-tidy] properly handle private move constructors in `modernize-pass-by-value` check (PR #141304)
Baranov Victor via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 9 02:53:15 PDT 2025
https://github.com/vbvictor updated https://github.com/llvm/llvm-project/pull/141304
>From 7323c1b3ed55790cdab240f233113bfb1d52badb Mon Sep 17 00:00:00 2001
From: Baranov Victor <bar.victor.2002 at gmail.com>
Date: Sat, 17 May 2025 18:40:54 +0300
Subject: [PATCH 1/5] [clang-tidy] Add isFriendOfClass helper function for
PassByValueCheck
---
.../clang-tidy/modernize/PassByValueCheck.cpp | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index 35f90fb8da15b..dd276b9e9be32 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -44,6 +44,18 @@ AST_MATCHER(CXXRecordDecl, isMoveConstructible) {
}
} // namespace
+static bool isFriendOfClass(const CXXRecordDecl *Class,
+ const CXXRecordDecl *Friend) {
+ 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;
+ });
+}
+
static TypeMatcher notTemplateSpecConstRefType() {
return lValueReferenceType(
pointee(unless(elaboratedType(namesType(templateSpecializationType()))),
>From 15b485a4f4a1726d49806d5cf6f8b2b54ec20c5a Mon Sep 17 00:00:00 2001
From: Baranov Victor <bar.victor.2002 at gmail.com>
Date: Sat, 17 May 2025 19:03:46 +0300
Subject: [PATCH 2/5] [clang-tidy] Bind outer class in constructor matcher
---
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp | 1 +
1 file changed, 1 insertion(+)
diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index dd276b9e9be32..454cf8e5be637 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -214,6 +214,7 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) {
traverse(
TK_AsIs,
cxxConstructorDecl(
+ ofClass(cxxRecordDecl().bind("outer")),
forEachConstructorInitializer(
cxxCtorInitializer(
unless(isBaseInitializer()),
>From 51b0b24e1c685fd292b8f98df000e68e05b2c854 Mon Sep 17 00:00:00 2001
From: Baranov Victor <bar.victor.2002 at gmail.com>
Date: Fri, 23 May 2025 18:10:09 +0300
Subject: [PATCH 3/5] [clang-tidy] add
'isMoveConstructibleInBoundCXXRecordDecl' matcher to properly handle private
move constructors in `pass-by-value` check
---
.../clang-tidy/modernize/PassByValueCheck.cpp | 64 ++++++++++++-------
clang-tools-extra/docs/ReleaseNotes.rst | 4 ++
.../checkers/modernize/pass-by-value.cpp | 61 ++++++++++++++++++
3 files changed, 107 insertions(+), 22 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index 454cf8e5be637..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,29 +45,35 @@ 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;
-}
-} // namespace
-
-static bool isFriendOfClass(const CXXRecordDecl *Class,
- const CXXRecordDecl *Friend) {
- return llvm::any_of(
- Class->friends(), [Friend](FriendDecl *FriendDecl) -> bool {
- if (TypeSourceInfo *FriendTypeSource = FriendDecl->getFriendType()) {
- const QualType FriendType = FriendTypeSource->getType();
- return FriendType->getAsCXXRecordDecl() == Friend;
+/// 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 false;
+ return true;
});
}
+} // namespace
static TypeMatcher notTemplateSpecConstRefType() {
return lValueReferenceType(
@@ -238,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 6856aa257d670..ad869265a2db5 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -281,6 +281,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-type-traits
<clang-tidy/checks/modernize/type-traits>` check by detecting more type traits.
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..72f8d41ea6550 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,64 @@ 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) {}
+ // CHECK-FIXES: X1(const ProtectedMovable &M) : M(M) {}
+ ProtectedMovable M;
+};
+
+struct X2 {
+ X2(const PrivateMovable &M) : M(M) {}
+ // CHECK-FIXES: 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;
+};
>From 7993aac14398d26d00b8b51277bc85aa79fb65f4 Mon Sep 17 00:00:00 2001
From: Baranov Victor <bar.victor.2002 at gmail.com>
Date: Sat, 24 May 2025 02:52:30 +0300
Subject: [PATCH 4/5] deleted unused check-fixes in tests
---
.../test/clang-tidy/checkers/modernize/pass-by-value.cpp | 2 --
1 file changed, 2 deletions(-)
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 72f8d41ea6550..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
@@ -283,13 +283,11 @@ struct InheritedPrivateMovable : PrivateMovable {
struct X1 {
X1(const ProtectedMovable &M) : M(M) {}
- // CHECK-FIXES: X1(const ProtectedMovable &M) : M(M) {}
ProtectedMovable M;
};
struct X2 {
X2(const PrivateMovable &M) : M(M) {}
- // CHECK-FIXES: X2(const PrivateMovable &M) : M(M) {}
PrivateMovable M;
};
>From 43148f20c6d31c8daa12b48dfa70704d05f618a2 Mon Sep 17 00:00:00 2001
From: Baranov Victor <bar.victor.2002 at gmail.com>
Date: Sat, 24 May 2025 02:57:45 +0300
Subject: [PATCH 5/5] apply clang-format
---
clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index 343cc966b7e09..1e271dfa768ce 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -34,7 +34,7 @@ static bool isFirstFriendOfSecond(const CXXRecordDecl *Friend,
namespace {
/// Matches move-constructible classes whose constructor can be called inside
-/// a CXXRecordDecl with a bound ID.
+/// a CXXRecordDecl with a bound ID.
///
/// Given
/// \code
@@ -49,7 +49,7 @@ namespace {
/// class Buz {
/// Buz(Buz &&);
/// int a;
-/// friend class Outer;
+/// friend class Outer;
/// };
///
/// class Outer {
More information about the cfe-commits
mailing list