[PATCH] D112648: [clang-tidy] Improve fix-its for `modernize-pass-by-value` check

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang updated this revision to Diff 391059.
avogelsgesang marked 4 inline comments as done.
avogelsgesang added a comment.

Address review comments


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D112648/new/

https://reviews.llvm.org/D112648

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


Index: clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
===================================================================
--- clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
+++ clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp
@@ -231,5 +231,18 @@
 
 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) {}
Index: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
===================================================================
--- clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
+++ clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp
@@ -190,24 +190,35 @@
 
   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>();
+  // 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>();
 
-    // 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);
+      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.


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D112648.391059.patch
Type: text/x-patch
Size: 3548 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20211201/5d5eb6c9/attachment.bin>