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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
