diff --git a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp index 2619331cba0b0..d3fba478216f1 100644 --- a/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp +++ b/clang-tools-extra/clang-tidy/modernize/PassByValueCheck.cpp @@ -84,10 +84,11 @@ static TypeMatcher nonConstValueType() { return qualType(unless(anyOf(referenceType(), isConstQualified()))); } -/// Whether or not \p ParamDecl is used exactly one time in \p Ctor. +/// Whether or not \p ParamDecl is used exactly one time in \p Func. /// -/// Checks both in the init-list and the body of the constructor. -static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, +/// Checks both in the init-list (for constructors) and the body of the +/// function. +static bool paramReferredExactlyOnce(const FunctionDecl *Func, const ParmVarDecl *ParamDecl) { /// \c clang::RecursiveASTVisitor that checks that the given /// \c ParmVarDecl is used exactly one time. @@ -102,11 +103,10 @@ static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, : ParamDecl(ParamDecl) {} /// Whether or not the parameter variable is referred only once in - /// the - /// given constructor. - bool hasExactlyOneUsageIn(const CXXConstructorDecl *Ctor) { + /// the given function. + bool hasExactlyOneUsageIn(const FunctionDecl *Func) { Count = 0U; - TraverseDecl(const_cast(Ctor)); + TraverseDecl(const_cast(Func)); return Count == 1U; } @@ -131,82 +131,82 @@ static bool paramReferredExactlyOnce(const CXXConstructorDecl *Ctor, unsigned Count = 0U; }; - return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Ctor); + return ExactlyOneUsageVisitor(ParamDecl).hasExactlyOneUsageIn(Func); } -/// Returns true if the given constructor is part of a lvalue/rvalue reference -/// pair, i.e. `Param` is of lvalue reference type, and there exists another -/// constructor such that: -/// - it has the same number of parameters as `Ctor`. -/// - the parameter at the same index as `Param` is an rvalue reference -/// of the same pointee type -/// - all other parameters have the same type as the corresponding parameter in -/// `Ctor` or are rvalue references with the same pointee type. -/// Examples: -/// A::A(const B& Param) -/// A::A(B&&) +/// Returns true if \p Func has a paired overload where the parameter at the +/// same index as \p Param is an rvalue reference of the same pointee type. /// -/// A::A(const B& Param, const C&) -/// A::A(B&& Param, C&&) +/// For constructors, only other constructors of the same class are checked. +/// For free/member functions, overloads are looked up in the same +/// DeclContext (namespace or class). /// -/// A::A(const B&, const C& Param) -/// A::A(B&&, C&& Param) -/// -/// A::A(const B&, const C& Param) -/// A::A(const B&, C&& Param) +/// Examples: +/// void foo(const B& Param) +/// void foo(B&&) /// -/// A::A(const B& Param, int) -/// A::A(B&& Param, int) -static bool hasRValueOverload(const CXXConstructorDecl *Ctor, +/// A::A(const B& Param) +/// A::A(B&&) +static bool hasRValueOverload(const FunctionDecl *Func, const ParmVarDecl *Param) { if (!Param->getType().getCanonicalType()->isLValueReferenceType()) { // The parameter is passed by value. return false; } const int ParamIdx = Param->getFunctionScopeIndex(); - const CXXRecordDecl *Record = Ctor->getParent(); - // Check whether a ctor `C` forms a pair with `Ctor` under the aforementioned - // rules. - const auto IsRValueOverload = [&Ctor, ParamIdx](const CXXConstructorDecl *C) { - if (C == Ctor || C->isDeleted() || - C->getNumParams() != Ctor->getNumParams()) + // Helper to check whether a candidate function forms an lvalue/rvalue pair. + const auto IsRValueOverload = [Func, + ParamIdx](const FunctionDecl *Candidate) { + if (Candidate == Func || Candidate->isDeleted() || + Candidate->getNumParams() != Func->getNumParams()) return false; - for (int I = 0, E = C->getNumParams(); I < E; ++I) { + for (int I = 0, E = Candidate->getNumParams(); I < E; ++I) { const clang::QualType CandidateParamType = - C->parameters()[I]->getType().getCanonicalType(); - const clang::QualType CtorParamType = - Ctor->parameters()[I]->getType().getCanonicalType(); + Candidate->parameters()[I]->getType().getCanonicalType(); + const clang::QualType FuncParamType = + Func->parameters()[I]->getType().getCanonicalType(); const bool IsLValueRValuePair = - CtorParamType->isLValueReferenceType() && + FuncParamType->isLValueReferenceType() && CandidateParamType->isRValueReferenceType() && CandidateParamType->getPointeeType()->getUnqualifiedDesugaredType() == - CtorParamType->getPointeeType()->getUnqualifiedDesugaredType(); + FuncParamType->getPointeeType()->getUnqualifiedDesugaredType(); if (I == ParamIdx) { // The parameter of interest must be paired. if (!IsLValueRValuePair) return false; } else { // All other parameters can be similar or paired. - if (!(CandidateParamType == CtorParamType || IsLValueRValuePair)) + if (!(CandidateParamType == FuncParamType || IsLValueRValuePair)) return false; } } return true; }; - return llvm::any_of(Record->ctors(), IsRValueOverload); + // For constructors, check sibling constructors. + if (const auto *Ctor = dyn_cast(Func)) { + const CXXRecordDecl *Record = Ctor->getParent(); + return llvm::any_of(Record->ctors(), IsRValueOverload); + } + + // For other functions, look up overloads in the same DeclContext. + const DeclContext *DC = Func->getDeclContext(); + const DeclarationName Name = Func->getDeclName(); + return llvm::any_of(DC->lookup(Name), [IsRValueOverload](const Decl *D) { + const auto *FD = dyn_cast(D); + return FD && IsRValueOverload(FD); + }); } /// Find all references to \p ParamDecl across all of the -/// redeclarations of \p Ctor. +/// redeclarations of \p Func. static SmallVector -collectParamDecls(const CXXConstructorDecl *Ctor, - const ParmVarDecl *ParamDecl) { +collectParamDecls(const FunctionDecl *Func, const ParmVarDecl *ParamDecl) { SmallVector Results; const unsigned ParamIdx = ParamDecl->getFunctionScopeIndex(); - for (const FunctionDecl *Redecl : Ctor->redecls()) + for (const FunctionDecl *Redecl : Func->redecls()) Results.push_back(Redecl->getParamDecl(ParamIdx)); return Results; } @@ -226,6 +226,7 @@ void PassByValueCheck::storeOptions(ClangTidyOptions::OptionMap &Opts) { } void PassByValueCheck::registerMatchers(MatchFinder *Finder) { + // Matcher for constructor member initializer lists (existing behavior). Finder->addMatcher( traverse( TK_AsIs, @@ -260,6 +261,32 @@ void PassByValueCheck::registerMatchers(MatchFinder *Finder) { .bind("Initializer"))) .bind("Ctor")), this); + + // Matcher for function body local variable copies from const-ref parameters. + // Matches patterns like: + // void f(const T& param) { T local = param; } + if (!ValuesOnly) { + Finder->addMatcher( + traverse( + TK_AsIs, + functionDecl( + unless(isInstantiated()), + hasBody(hasDescendant( + varDecl( + hasLocalStorage(), + hasInitializer(ignoringImplicit(cxxConstructExpr( + hasDeclaration(cxxConstructorDecl( + isCopyConstructor(), unless(isDeleted()))), + hasArgument( + 0, + ignoringImplicit(declRefExpr(to( + parmVarDecl( + hasType(notTemplateSpecConstRefType())) + .bind("FuncParam"))))))))) + .bind("LocalVar")))) + .bind("Func")), + this); + } } void PassByValueCheck::registerPPCallbacks(const SourceManager &SM, @@ -268,71 +295,138 @@ void PassByValueCheck::registerPPCallbacks(const SourceManager &SM, Inserter.registerPreprocessor(PP); } +/// Attempts to rewrite the const-ref parameter declarations to pass-by-value +/// across all redeclarations. Returns true if fixits were added, false if +/// rewriting is not possible (e.g. type hidden behind a typedef). +static bool rewriteParamDeclsToValue(const FunctionDecl *Func, + const ParmVarDecl *ParamDecl, + const SourceManager &SM, + const LangOptions &LangOpts, + DiagnosticBuilder &Diag) { + if (!ParamDecl->getType()->isLValueReferenceType()) + return true; // Already by value, nothing to rewrite. + + // Check if we can successfully rewrite all declarations. + for (const ParmVarDecl *ParmDecl : collectParamDecls(Func, ParamDecl)) { + const TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); + auto RefTL = ParamTL.getAs(); + 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 false; + } + } + // Rewrite all declarations. + for (const ParmVarDecl *ParmDecl : collectParamDecls(Func, ParamDecl)) { + const TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); + auto RefTL = ParamTL.getAs(); + + const TypeLoc ValueTL = RefTL.getPointeeLoc(); + const CharSourceRange TypeRange = CharSourceRange::getTokenRange( + ParmDecl->getBeginLoc(), ParamTL.getEndLoc()); + std::string ValueStr = Lexer::getSourceText(CharSourceRange::getTokenRange( + ValueTL.getSourceRange()), + SM, LangOpts) + .str(); + ValueStr += ' '; + Diag << FixItHint::CreateReplacement(TypeRange, ValueStr); + } + return true; +} + void PassByValueCheck::check(const MatchFinder::MatchResult &Result) { - const auto *Ctor = Result.Nodes.getNodeAs("Ctor"); - const auto *ParamDecl = Result.Nodes.getNodeAs("Param"); - const auto *Initializer = - Result.Nodes.getNodeAs("Initializer"); const SourceManager &SM = *Result.SourceManager; + // Case 1: Constructor member initializer list. + if (const auto *Initializer = + Result.Nodes.getNodeAs("Initializer")) { + const auto *Ctor = Result.Nodes.getNodeAs("Ctor"); + const auto *ParamDecl = Result.Nodes.getNodeAs("Param"); + + if (IgnoreMacros && ParamDecl->getBeginLoc().isMacroID()) + return; + + if (!paramReferredExactlyOnce(Ctor, ParamDecl)) + return; + + if (ParamDecl->getType().getNonReferenceType().isTriviallyCopyableType( + *Result.Context)) + return; + + if (hasRValueOverload(Ctor, ParamDecl)) + return; + + auto Diag = + diag(ParamDecl->getBeginLoc(), "pass by value and use std::move"); + + if (!rewriteParamDeclsToValue(Ctor, ParamDecl, SM, getLangOpts(), Diag)) + return; + + // Use std::move in the initialization list. + Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")") + << FixItHint::CreateInsertion( + Initializer->getLParenLoc().getLocWithOffset(1), "std::move(") + << Inserter.createIncludeInsertion( + Result.SourceManager->getFileID( + Initializer->getSourceLocation()), + ""); + return; + } + + // Case 2: Function body local variable copy. + const auto *Func = Result.Nodes.getNodeAs("Func"); + const auto *ParamDecl = Result.Nodes.getNodeAs("FuncParam"); + const auto *LocalVar = Result.Nodes.getNodeAs("LocalVar"); + if (!Func || !ParamDecl || !LocalVar) + return; + if (IgnoreMacros && ParamDecl->getBeginLoc().isMacroID()) return; - // If the parameter is used or anything other than the copy, do not apply - // the changes. - if (!paramReferredExactlyOnce(Ctor, ParamDecl)) + if (!paramReferredExactlyOnce(Func, ParamDecl)) return; - // If the parameter is trivial to copy, don't move it. Moving a trivially - // copyable type will cause a problem with performance-move-const-arg if (ParamDecl->getType().getNonReferenceType().isTriviallyCopyableType( *Result.Context)) return; - // Do not trigger if we find a paired constructor with an rvalue. - if (hasRValueOverload(Ctor, ParamDecl)) + if (hasRValueOverload(Func, ParamDecl)) + return; + + // Check that the copied type has a usable move constructor. + const QualType LocalType = LocalVar->getType().getCanonicalType(); + const CXXRecordDecl *Record = LocalType->getAsCXXRecordDecl(); + if (!Record || !Record->hasDefinition()) + return; + bool HasUsableMove = false; + for (const CXXConstructorDecl *Ctor : Record->ctors()) + if (Ctor->isMoveConstructor() && !Ctor->isDeleted()) { + HasUsableMove = true; + break; + } + if (!HasUsableMove && !Record->needsImplicitMoveConstructor()) return; auto Diag = diag(ParamDecl->getBeginLoc(), "pass by value and use std::move"); - // 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)) { - const TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); - auto RefTL = ParamTL.getAs(); - 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)) { - const TypeLoc ParamTL = ParmDecl->getTypeSourceInfo()->getTypeLoc(); - auto RefTL = ParamTL.getAs(); - - const TypeLoc ValueTL = RefTL.getPointeeLoc(); - const 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); + if (!rewriteParamDeclsToValue(Func, ParamDecl, SM, getLangOpts(), Diag)) + return; + + // Wrap the initializer with std::move(). + const Expr *Init = LocalVar->getInit()->IgnoreImplicit(); + if (const auto *Construct = dyn_cast(Init)) { + if (Construct->getNumArgs() > 0) { + const Expr *Arg = Construct->getArg(0)->IgnoreImplicit(); + Diag << FixItHint::CreateInsertion(Arg->getBeginLoc(), "std::move(") + << FixItHint::CreateInsertion( + Lexer::getLocForEndOfToken(Arg->getEndLoc(), 0, SM, + getLangOpts()), + ")"); } } - // Use std::move in the initialization list. - Diag << FixItHint::CreateInsertion(Initializer->getRParenLoc(), ")") - << FixItHint::CreateInsertion( - Initializer->getLParenLoc().getLocWithOffset(1), "std::move(") - << Inserter.createIncludeInsertion( - Result.SourceManager->getFileID(Initializer->getSourceLocation()), - ""); + Diag << Inserter.createIncludeInsertion( + Result.SourceManager->getFileID(LocalVar->getLocation()), ""); } } // namespace clang::tidy::modernize diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 68bab88146241..3601a26ed8cd7 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -207,7 +207,9 @@ Changes in existing checks - Improved :doc:`modernize-pass-by-value ` check by adding `IgnoreMacros` - option to suppress warnings in macros. + option to suppress warnings in macros. Also extended the check to handle + ``const T&`` function parameters that are copied into local variables in + function bodies, not just constructor member initializer lists. - Improved :doc:`modernize-use-std-format ` check by fixing a crash diff --git a/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst b/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst index 0f5758dc097e6..8b8810958a784 100644 --- a/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/modernize/pass-by-value.rst @@ -23,12 +23,6 @@ example illustrates how the construction of the value happens: foo(get_str()); // prvalue -> move construction } -.. note:: - - Currently, only constructors are transformed to make use of pass-by-value. - Contributions that handle other situations are welcome! - - Pass-by-value in constructors ----------------------------- @@ -85,11 +79,43 @@ untouched: }; +Pass-by-value in function bodies +--------------------------------- + +Replaces ``const T&`` function parameters that are unconditionally copied into +a local variable with pass-by-value and ``std::move()``. This applies to free +functions, member functions, and constructors (for copies in the body rather +than the initializer list). + +.. code-block:: c++ + + #include + + -void process(const std::string &S) { + - std::string Local = S; + +void process(std::string S) { + + std::string Local = std::move(S); + // use Local... + } + +The check only triggers when: + +- The parameter type is a non-trivially-copyable class or struct. +- The type has a non-deleted move constructor. +- The parameter is used exactly once (in the copy). +- There is no overload taking an rvalue reference for the same parameter. + +.. note:: + + This transformation is not applied when :option:`ValuesOnly` is set to + `true`, or in dependent (template) contexts. + Limitations ----------- -A situation where the generated code can be wrong is when the object referenced -is modified before the assignment in the init-list through a "hidden" reference. +A situation where the generated code can be wrong is when the object +referenced is modified before the assignment in the init-list through a +"hidden" reference. Example: diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value-local-copy.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value-local-copy.cpp new file mode 100644 index 0000000000000..f6eb5fc98090a --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/pass-by-value-local-copy.cpp @@ -0,0 +1,114 @@ +// RUN: %check_clang_tidy %s modernize-pass-by-value %t -- -- -fno-delayed-template-parsing + +namespace std { +template +struct remove_reference { typedef T type; }; +template +struct remove_reference { typedef T type; }; +template +struct remove_reference { typedef T type; }; + +template +typename remove_reference::type &&move(T &&) noexcept; +} // namespace std + +struct Movable { + int a, b, c; + Movable() = default; + Movable(const Movable &) {} + Movable(Movable &&) {} +}; + +struct NotMovable { + NotMovable() = default; + NotMovable(const NotMovable &) = default; + NotMovable(NotMovable &&) = delete; + int a, b, c; +}; + +// POD types are trivially move constructible. +struct POD { + int a, b, c; +}; + +// Positive: const string-like ref copied into local variable in free function. +void take_movable(const Movable &M) { + // CHECK-MESSAGES: :[[@LINE-1]]:19: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: void take_movable(Movable M) { + Movable Local = M; + // CHECK-FIXES: Movable Local = std::move(M); + (void)Local; +} + +// Positive: const ref copied in a method. +struct MyClass { + void process(const Movable &M) { + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: void process(Movable M) { + Movable Copy = M; + // CHECK-FIXES: Movable Copy = std::move(M); + (void)Copy; + } +}; + +// Positive: separate declaration and definition. +void with_decl(const Movable &M); +// CHECK-FIXES: void with_decl(Movable M); +void with_decl(const Movable &M) { + // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: pass by value and use std::move [modernize-pass-by-value] + // CHECK-FIXES: void with_decl(Movable M) { + Movable Local = M; + // CHECK-FIXES: Movable Local = std::move(M); + (void)Local; +} + +// Negative: trivially copyable type. +void take_pod(const POD &P) { + POD Local = P; + (void)Local; +} + +// Negative: no copy, just using the reference. +void use_ref(const Movable &M) { + (void)M.a; +} + +// Negative: type has no move constructor (deleted). +void take_nomove(const NotMovable &N) { + NotMovable Local = N; + (void)Local; +} + +// Negative: parameter used more than once. +void multi_use(const Movable &M) { + Movable Local = M; + (void)Local; + (void)M.a; +} + +// Negative: parameter is not const ref. +void take_value(Movable M) { + Movable Local = M; + (void)Local; +} + +// Negative: not a parameter, just a local. +void local_copy() { + Movable A; + Movable B = A; + (void)B; +} + +// Negative: rvalue overload exists. +void with_rvalue_overload(const Movable &M) { + Movable Local = M; + (void)Local; +} +void with_rvalue_overload(Movable &&M); + +// Negative: template function (dependent context). +template +void take_template(const T &M) { + T Local = M; + (void)Local; +}