avogelsgesang created this revision.
Herald added subscribers: carlosgalvezp, xazax.hun.
avogelsgesang requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
This commit improves the fix-its of `modernize-pass-by-value` in two ways:
1. No longer propose 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
2. The check unconditionally added a whitespace after the new type name. This
is necessary, e.g., for LLVM's code style where the `&` is directly attached to
the variable name. For other code styles, we don't want this whitespace,
though. The check now checks if a whitespace is already present and if so
doesn't introduce a 2nd duplicated whitespace
Repository:
rG LLVM Github Monorepo
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,33 @@
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) {}
+
+// Check that we don't insert an unnecessary whitespace if not required
+struct W {
+ // Here we need a whitespace so we don't write `MovableM`
+ W(const Movable &M);
+ // CHECK-FIXES: W(Movable M);
+ Movable M;
+};
+// Here we don't need an additional whitepace, as there already is a whitespace
+// between `&` and the variable name
+// clang-format off
+W::W(const Movable& M) : M(M) {}
+// CHECK-MESSAGES: :[[@LINE-1]]:6: warning: pass by value and use std::move
+// CHECK-FIXES: W::W(Movable M) : M(std::move(M)) {}
+// clang-format on
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,46 @@
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)) {
+ auto ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc();
+ auto 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)) {
+ 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();
- 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 =
+ Lexer::getLocForEndOfToken(ParamTL.getEndLoc(), 0, SM, getLangOpts());
+ auto NextChar = Lexer::getSourceText(
+ CharSourceRange::getCharRange(TypeRangeEnd,
+ TypeRangeEnd.getLocWithOffset(1)),
+ SM, getLangOpts());
+ if (NextChar != " ") {
+ ValueStr += ' ';
+ }
+
+ Diag << FixItHint::CreateReplacement(TypeRange, ValueStr);
+ }
}
// Use std::move in the initialization list.
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
