-
Notifications
You must be signed in to change notification settings - Fork 13.6k
[clang-tidy] properly handle private move constructors in modernize-pass-by-value
check
#141304
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
… properly handle private move constructors in `pass-by-value` check
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Baranov Victor (vbvictor) ChangesFixed 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:
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;
+};
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
hasDeclContext( | ||
cxxRecordDecl(isMoveConstructible()))))))) | ||
hasDeclContext(cxxRecordDecl( | ||
isMoveConstructibleInBoundCXXRecordDecl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this could be checked in line 236, we got there record so just check if move constructor is public..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider this code:
struct Movable {
int a, b, c;
Movable() = default;
Movable(const Movable &) {}
Movable(Movable &&) {}
};
struct A {
A(const Movable &M) : M(M) {}
Movable M;
};
In line 236 we bind struct A
to outer
so checking it has public move constructor has no use for us.
We need to check that struct Movable
has move constructor and this is checked in line 261.
Alternatively, I can write here something like
hasDeclContext(cxxRecordDecl(
has(cxxConstructorDecl(
isMoveConstructor(),
anyOf(
isPublic(),
isFriendOf("outer")) // if we have private ctor but we can still call it because of `friend`
)))
))
but this logic is placed inside isMoveConstructibleInBoundCXXRecordDecl()
Fixed false positives when class passed by const-reference had a private move constructor, which could not be used for a fix-it.
Closes #140236.