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

[courbet]

Summary

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

[flx]

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

108

if

160

A comment why we have to have a different check for when the params are not at ParamIdx would be helpful.

[courbet]

Thanks.

[courbet]

Address review comments.

clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp

private:
 
     const ParmVarDecl *ParamDecl;
     unsigned Count;
   };
 
   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>
 collectParamDecls(const CXXConstructorDecl *Ctor,
                   const ParmVarDecl *ParamDecl) {
   SmallVector<const ParmVarDecl *, 2> Results;
   unsigned ParamIdx = ParamDecl->getFunctionScopeIndex();
 
if (!paramReferredExactlyOnce(Ctor, ParamDecl))
     return;
 
   // If the parameter is trivial to copy, don't move it. Moving a trivially
   // copyable type will cause a problem with performance-move-const-arg
   if (ParamDecl->getType().getNonReferenceType().isTriviallyCopyableType(
           *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
   // declarations.
   if (ParamDecl->getType()->isLValueReferenceType()) {
     // Check if we can succesfully rewrite all declarations of the constructor.
     for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
       TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();

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

 struct V {
   V(MovableConstRef M);
   // CHECK-FIXES: V(MovableConstRef M);
   Movable M;
 };
 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;
+};