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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Thanks for your patience!



================
Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:198
+    for (const ParmVarDecl *ParmDecl : collectParamDecls(Ctor, ParamDecl)) {
+      auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+      auto RefTL = ParamTL.getAs<ReferenceTypeLoc>();
----------------
Please spell out this type (per our usual coding conventions). The use on the next line is fine because the type name is spelled out in the initialization.


================
Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:220
 
-    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);
+      // Check if we need to append an additional whitespace
+      auto TypeRangeEnd =
----------------
I don't think we should take these changes, unless they're strictly necessary for program correctness. The rule of thumb in clang-tidy for fix-its is that we do not make much attempt to have pretty formatting from the fix (the fix needs to be correct, but doesn't need to be formatted); the user can run clang-format after applying fixes. Someday, we'd like to consume clang-format as a library and automatically reformat the code after applying fixes instead of trying to catch every formatting concern in each individual check.


================
Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:221-225
+      auto TypeRangeEnd =
+          Lexer::getLocForEndOfToken(ParamTL.getEndLoc(), 0, SM, getLangOpts());
+      auto NextChar = Lexer::getSourceText(
+          CharSourceRange::getCharRange(TypeRangeEnd,
+                                        TypeRangeEnd.getLocWithOffset(1)),
----------------
Please spell out the types.


================
Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:227-229
+      if (NextChar != " ") {
+        ValueStr += ' ';
+      }
----------------
(Style guideline nit)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648