[clang-tools-extra] 5d3b895 - Don't offer partial fix-its for `modernize-pass-by-value`

[Aaron Ballman via cfe-commits]

Author: Adrian Vogelsgesang
Date: 2021-12-08T13:31:30-05:00
New Revision: 5d3b8956e83484ff7234dda5e939abbdc9bd2c88

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

LOG: Don't offer partial fix-its for `modernize-pass-by-value`

This commit improves the fix-its of modernize-pass-by-value by
no longer proposing partial fixes. In the presence of using/typedef,
we failed to rewrite the function signature but still adjusted the
function body. This led to incorrect, partial fix-its. Instead, the
check now simply doesn't offer any fixes at all in such a situation.

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 3caaa3c876ab3..34079c65b68cb 100644
--- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -190,24 +190,35 @@ void PassByValueCheck::check(const MatchFinder::MatchResult &Result) {
 
   auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
 
-  // Iterate over all declarations of the constructor.
-  for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
-    auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
-    auto RefTL = ParamTL.getAs<ReferenceTypeLoc>();
-
-    // Do not replace if it is already a value, skip.
-    if (RefTL.isNull())
-      continue;
-
-    TypeLoc ValueTL = RefTL.getPointeeLoc();
-    auto TypeRange = CharSourceRange::getTokenRange(ParmDecl->getBeginLoc(),
-                                                    ParamTL.getEndLoc());
-    std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange(
-                                                    ValueTL.getSourceRange()),
-                                                SM, getLangOpts())
-                               .str();
-    ValueStr += ' ';
-    Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+  // 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();
+      ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+      if (RefTL.isNull()) {
+        // We cannot rewrite this instance. The type is probably hidden behind
+        // some `typedef`. Do not offer a fix-it in this case.
+        return;
+      }
+    }
+    // Rewrite all declarations.
+    for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+      TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+      ReferenceTypeLoc RefTL = ParamTL.getAs<ReferenceTypeLoc>();
+
+      TypeLoc ValueTL = RefTL.getPointeeLoc();
+      CharSourceRange TypeRange = CharSourceRange::getTokenRange(
+          ParmDecl->getBeginLoc(), ParamTL.getEndLoc());
+      std::string ValueStr =
+          Lexer::getSourceText(
+              CharSourceRange::getTokenRange(ValueTL.getSourceRange()), SM,
+              getLangOpts())
+              .str();
+      ValueStr += ' ';
+      Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+    }
   }
 
   // Use std::move in the initialization list.

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 45f51a902cc36..28e4014c43d36 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
@@ -231,5 +231,18 @@ struct T {
 
 struct U {
   U(const POD &M) : M(M) {}
+  // CHECK-FIXES: U(const POD &M) : M(M) {}
   POD M;
 };
+
+// The rewrite can't look through `typedefs` and `using`.
+// Test that we don't partially rewrite one decl without rewriting the other.
+using MovableConstRef = const Movable &;
+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) {}