Author: Adrian Vogelsgesang Date: 2021-12-08T13:31:30-05:00 New Revision: 5d3b8956e83484ff7234dda5e939abbdc9bd2c88
URL: https://github.com/llvm/llvm-project/commit/5d3b8956e83484ff7234dda5e939abbdc9bd2c88 DIFF: https://github.com/llvm/llvm-project/commit/5d3b8956e83484ff7234dda5e939abbdc9bd2c88.diff LOG: Don't offer partial fix-its for `modernize-pass-by-value` This commit improves the fix-its of modernize-pass-by-value by no longer proposing 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. Added: Modified: clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp Removed: ################################################################################ diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp index 3caaa3c876ab3..34079c65b68cb 100644 --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp @@ -190,24 +190,35 @@ void PassByValueCheck::check(const MatchFinder::MatchResult &Result) { 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>(); - - // 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); + // 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>(); + + 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. diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp index 45f51a902cc36..28e4014c43d36 100644 --- a/clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize-pass-by-value.cpp @@ -231,5 +231,18 @@ struct T { 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) {} _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits