[clang-tools-extra] ed8ff29 - [clang-tidy] Fix false positive in modernize-pass-by-value

[Clement Courbet via cfe-commits]

Author: Clement Courbet
Date: 2022-01-05T14:33:40+01:00
New Revision: ed8ff29aa6835187f3d5e7b64de022fe6b33a131

URL: https://github.com/llvm/llvm-project/commit/ed8ff29aa6835187f3d5e7b64de022fe6b33a131
DIFF: https://github.com/llvm/llvm-project/commit/ed8ff29aa6835187f3d5e7b64de022fe6b33a131.diff

LOG: [clang-tidy] Fix false positive in modernize-pass-by-value

The check should not trigger on lvalue/rvalue overload pairs:

```
struct S {
  S(const A& a) : a(a) {}
  S(A&& a) : a(std::move(a)) {}

  A a;
}
```

Differential Revision: https://reviews.llvm.org/D116535

Added: 
    

Modified: 
    clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
    clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp

Removed: 
    


################################################################################
diff  --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
index 34079c65b68cb..2e1f2143947e9 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -105,6 +105,75 @@ static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor,
   return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor);
 }
 
+/// Returns true if the given constructor is part of a lvalue/rvalue reference
+/// pair, i.e. `Param` is of lvalue reference type, and there exists another
+/// constructor such that:
+///  - it has the same number of parameters as `Ctor`.
+///  - the parameter at the same index as `Param` is an rvalue reference
+///    of the same pointee type
+///  - all other parameters have the same type as the corresponding parameter in
+///    `Ctor` or are rvalue references with the same pointee type.
+/// Examples:
+///  A::A(const B& Param)
+///  A::A(B&&)
+///
+///  A::A(const B& Param, const C&)
+///  A::A(B&& Param, C&&)
+///
+///  A::A(const B&, const C& Param)
+///  A::A(B&&, C&& Param)
+///
+///  A::A(const B&, const C& Param)
+///  A::A(const B&, C&& Param)
+///
+///  A::A(const B& Param, int)
+///  A::A(B&& Param, int)
+static bool hasRValueOverload(const CXXConstructorDecl *Ctor,
+                              const ParmVarDecl *Param) {
+  if (!Param->getType().getCanonicalType()->isLValueReferenceType()) {
+    // The parameter is passed by value.
+    return false;
+  }
+  const int ParamIdx = Param->getFunctionScopeIndex();
+  const CXXRecordDecl *Record = Ctor->getParent();
+
+  // Check whether a ctor `C` forms a pair with `Ctor` under the aforementionned
+  // rules.
+  const auto IsRValueOverload = [&Ctor, ParamIdx](const CXXConstructorDecl *C) {
+    if (C == Ctor || C->isDeleted() ||
+        C->getNumParams() != Ctor->getNumParams())
+      return false;
+    for (int I = 0, E = C->getNumParams(); I < E; ++I) {
+      const clang::QualType CandidateParamType =
+          C->parameters()[I]->getType().getCanonicalType();
+      const clang::QualType CtorParamType =
+          Ctor->parameters()[I]->getType().getCanonicalType();
+      const bool IsLValueRValuePair =
+          CtorParamType->isLValueReferenceType() &&
+          CandidateParamType->isRValueReferenceType() &&
+          CandidateParamType->getPointeeType()->getUnqualifiedDesugaredType() ==
+              CtorParamType->getPointeeType()->getUnqualifiedDesugaredType();
+      if (I == ParamIdx) {
+        // The parameter of interest must be paired.
+        if (!IsLValueRValuePair)
+          return false;
+      } else {
+        // All other parameters can be similar or paired.
+        if (!(CandidateParamType == CtorParamType || IsLValueRValuePair))
+          return false;
+      }
+    }
+    return true;
+  };
+
+  for (const auto *Candidate : Record->ctors()) {
+    if (IsRValueOverload(Candidate)) {
+      return true;
+    }
+  }
+  return false;
+}
+
 /// Find all references to \p ParamDecl across all of the
 /// redeclarations of \p Ctor.
 static SmallVector<const ParmVarDecl *, 2>
@@ -188,6 +257,10 @@ void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
           *Result.Context))
     return;
 
+  // Do not trigger if we find a paired constructor with an rvalue.
+  if (hasRValueOverload(Ctor, ParamDecl))
+    return;
+
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
 
   // If we received a `const&` type, we need to rewrite the function

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 28e4014c43d36..2aacbdd1c7a6a 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
@@ -246,3 +246,20 @@ struct V {
 V::V(const Movable &M) : M(M) {}
 // CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
 // CHECK-FIXES: V::V(const Movable &M) : M(M) {}
+
+// Test with paired lvalue/rvalue overloads.
+struct W1 {
+  W1(const Movable &M) : M(M) {}
+  W1(Movable &&M);
+  Movable M;
+};
+struct W2 {
+  W2(const Movable &M, int) : M(M) {}
+  W2(Movable &&M, int);
+  Movable M;
+};
+struct W3 {
+  W3(const W1 &, const Movable &M) : M(M) {}
+  W3(W1 &&, Movable &&M);
+  Movable M;
+};