Skip to content

[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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vbvictor
Copy link
Contributor

@vbvictor vbvictor commented May 23, 2025

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.

@llvmbot
Copy link
Member

llvmbot commented May 23, 2025

@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clang-tidy

Author: Baranov Victor (vbvictor)

Changes

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 --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;
+};

Copy link

github-actions bot commented May 23, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

hasDeclContext(
cxxRecordDecl(isMoveConstructible())))))))
hasDeclContext(cxxRecordDecl(
isMoveConstructibleInBoundCXXRecordDecl(
Copy link
Member

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..

Copy link
Contributor Author

@vbvictor vbvictor May 25, 2025

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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[clang-tidy] "modernize-pass-by-value" - should ignore move constructors from protected/private section
3 participants