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

[Adrian Vogelsgesang via Phabricator via cfe-commits]

avogelsgesang added inline comments.


================
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>();
----------------
aaron.ballman wrote:
> 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.
Thanks for the hint. I wasn't aware of that convention. I fixed it here.

Should I also fix the other violations in this file?

E.g., a couple of lines above, we have

```
auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
```

which I guess should be

```
clang::DiagnosticBuilder Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move");
```


================
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 =
----------------
aaron.ballman wrote:
> 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.
sounds good. Removed the whitespace related parts from this patch


================
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)),
----------------
aaron.ballman wrote:
> Please spell out the types.
no longer applicable after removing the whitespace related part of this patch


================
Comment at: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp:227-229
+      if (NextChar != " ") {
+        ValueStr += ' ';
+      }
----------------
aaron.ballman wrote:
> (Style guideline nit)
no longer applicable after removing the whitespace related part of this patch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112648