Index: clang-tidy/performance/CMakeLists.txt =================================================================== --- clang-tidy/performance/CMakeLists.txt +++ clang-tidy/performance/CMakeLists.txt @@ -5,6 +5,7 @@ ForRangeCopyCheck.cpp ImplicitCastInLoopCheck.cpp PerformanceTidyModule.cpp + ReturnValueCopyCheck.cpp UnnecessaryCopyInitialization.cpp UnnecessaryValueParamCheck.cpp Index: clang-tidy/performance/PerformanceTidyModule.cpp =================================================================== --- clang-tidy/performance/PerformanceTidyModule.cpp +++ clang-tidy/performance/PerformanceTidyModule.cpp @@ -10,6 +10,7 @@ #include "../ClangTidy.h" #include "../ClangTidyModule.h" #include "../ClangTidyModuleRegistry.h" +#include "ReturnValueCopyCheck.h" #include "FasterStringFindCheck.h" #include "ForRangeCopyCheck.h" @@ -30,6 +31,8 @@ "performance-for-range-copy"); CheckFactories.registerCheck( "performance-implicit-cast-in-loop"); + CheckFactories.registerCheck( + "performance-return-value-copy"); CheckFactories.registerCheck( "performance-unnecessary-copy-initialization"); CheckFactories.registerCheck( Index: clang-tidy/performance/ReturnValueCopyCheck.h =================================================================== --- /dev/null +++ clang-tidy/performance/ReturnValueCopyCheck.h @@ -0,0 +1,44 @@ +//===--- ReturnValueCopyCheck.h - clang-tidy-----------------------*- C++ -*-===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_RETURN_VALUE_COPY_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_RETURN_VALUE_COPY_H + +#include "../ClangTidy.h" +#include "../utils/IncludeInserter.h" + +namespace clang { +namespace tidy { +namespace performance { + +/// This check finds places where we are returning object of a different type than +/// the function return type. In such places, we should use std::move, otherwise +/// the object will not be moved automatically. +/// Check adds std::move if it could be beneficial. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/performance-return-value-copy.html +class ReturnValueCopyCheck : public ClangTidyCheck { +public: + ReturnValueCopyCheck(StringRef Name, ClangTidyContext *Context); + + void registerPPCallbacks(CompilerInstance &Compiler) override; + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + +private: + std::unique_ptr Inserter; + const utils::IncludeSorter::IncludeStyle IncludeStyle; +}; + +} // namespace performance +} // namespace tidy +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_PERFORMANCE_RETURN_VALUE_COPY_H Index: clang-tidy/performance/ReturnValueCopyCheck.cpp =================================================================== --- /dev/null +++ clang-tidy/performance/ReturnValueCopyCheck.cpp @@ -0,0 +1,260 @@ +//===--- ReturnValueCopyCheck.cpp - clang-tidy-----------------------------===// +// +// The LLVM Compiler Infrastructure +// +// This file is distributed under the University of Illinois Open Source +// License. See LICENSE.TXT for details. +// +//===----------------------------------------------------------------------===// + +#include "ReturnValueCopyCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/Preprocessor.h" + +using namespace clang::ast_matchers; +using namespace llvm; + +namespace clang { +namespace tidy { +namespace performance { + +namespace { + +/// \brief Matches if the construct expression's callee's declaration matches +/// the given matcher. +/// +/// Given +/// \code +/// // POD types are trivially move constructible. +/// struct Foo { +/// Foo() = default; +/// Foo(int i) { }; +/// }; +/// +/// Foo a; +/// Foo b(1); +/// +/// \endcode +/// ctorCallee(parameterCountIs(1)) +/// matches "b(1)". +AST_MATCHER_P(CXXConstructExpr, ctorCallee, + ast_matchers::internal::Matcher, + InnerMatcher) { + const CXXConstructorDecl *CtorDecl = Node.getConstructor(); + return (CtorDecl != nullptr && + InnerMatcher.matches(*CtorDecl, Finder, Builder)); +} + +/// \brief Matches QualType which after removing const and reference +/// matches the given matcher. +AST_MATCHER_P(QualType, ignoringRefsAndConsts, + ast_matchers::internal::Matcher, InnerMatcher) { + if (Node.isNull()) + return false; + + QualType Unqualified = Node.getNonReferenceType(); + Unqualified.removeLocalConst(); + return InnerMatcher.matches(Unqualified, Finder, Builder); +} + +/// \brief Matches ParmVarDecls which have default arguments. +AST_MATCHER(ParmVarDecl, hasDefaultArgument) { return Node.hasDefaultArg(); } + +/// \brief Matches function declarations which have all arguments defaulted +/// except first. +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, + hasOneActiveArgument) { + return anyOf(parameterCountIs(1), + allOf(unless(parameterCountIs(0)), + hasParameter(1, hasDefaultArgument()))); +} + +/// \brief Matches declarations of template type which matches the given +/// matcher. +AST_MATCHER_P(TemplateTypeParmDecl, hasTemplateType, + ast_matchers::internal::Matcher, InnerMatcher) { + const QualType TemplateType = + Node.getTypeForDecl()->getCanonicalTypeInternal(); + return InnerMatcher.matches(TemplateType, Finder, Builder); +} + +/// \brief Matches constructor declarations which are templates and can be used +/// to move-construct from any type. +AST_MATCHER_FUNCTION(ast_matchers::internal::Matcher, + moveConstructorFromAnyType) { + // Potentially danger: this matcher binds a name, with probably + // mean that you cant use it twice in your check! + const char TemplateArgument[] = "templateArgument"; + return cxxConstructorDecl( + hasParent(functionTemplateDecl(has(templateTypeParmDecl( + hasTemplateType(qualType().bind(TemplateArgument)))))), + hasOneActiveArgument(), + hasParameter( + 0, hasType(hasCanonicalType(allOf( + rValueReferenceType(), + ignoringRefsAndConsts(equalsBoundNode(TemplateArgument))))))); +} + +/// \brief Matches to qual types which are cxx records and has constructors that +/// matches the given matcher. +AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher, + hasConstructor, + ast_matchers::internal::Matcher, + InnerMatcher) { + return hasDeclaration( + cxxRecordDecl(hasDescendant(cxxConstructorDecl(InnerMatcher)))); +} + +/// \brief Matches to qual types which has constructors from type that matches +/// the given matcher. +AST_MATCHER_FUNCTION_P(ast_matchers::internal::Matcher, + hasConstructorFromType, + ast_matchers::internal::Matcher, + InnerMatcher) { + auto ConstructorMatcher = cxxConstructorDecl( + unless(isDeleted()), hasOneActiveArgument(), + hasParameter(0, hasType(hasCanonicalType(qualType(InnerMatcher))))); + + return hasConstructor(ConstructorMatcher); +} + +} // namespace + +void ReturnValueCopyCheck::registerMatchers(MatchFinder *Finder) { + if (!getLangOpts().CPlusPlus11) + return; + + // Matches to type with after ignoring refs and consts is the same as + // "constructedType" + auto HasTypeSameAsConstructed = hasType(hasCanonicalType( + ignoringRefsAndConsts(equalsBoundNode("constructedType")))); + + auto HasRvalueReferenceType = + hasType(hasCanonicalType(qualType(rValueReferenceType()))); + + auto RefOrConstVarDecl = varDecl(hasType( + hasCanonicalType(qualType(anyOf(referenceType(), isConstQualified()))))); + + // Matches to expression expression that have declaration with is reference or + // const + auto IsDeclaredAsRefOrConstType = + anyOf(hasDescendant(declRefExpr(to(RefOrConstVarDecl))), + declRefExpr(to(RefOrConstVarDecl))); + + // Constructor parameter must not already be rreference + auto ParameterMatcher = + hasType(hasCanonicalType(qualType(unless(rValueReferenceType())))); + + // Constructor argument must + // * have different type than constructed type + // * not be r value reference type + // * not be declared as const or as reference to other variable + // * not be temporary object + // * not be cast from temporary object + auto ArgumentMatcher = + unless(anyOf(HasTypeSameAsConstructed, HasRvalueReferenceType, + IsDeclaredAsRefOrConstType, cxxTemporaryObjectExpr(), + hasDescendant(cxxTemporaryObjectExpr()))); + + // Matches to type constructible from argumentCanonicalType type + // * by r reference or + // * by value, and argumentCanonicalType is move constructible + auto IsMoveOrCopyConstructibleFromParam = hasConstructorFromType(anyOf( + allOf(rValueReferenceType(), + ignoringRefsAndConsts(equalsBoundNode("argumentCanonicalType"))), + allOf(equalsBoundNode("argumentCanonicalType"), + hasConstructor(isMoveConstructor())))); + + // Matches to type that has template constructor which is + // move constructor from any type (like boost::any) + auto IsCopyConstructibleFromParamViaTemplate = + hasConstructor(moveConstructorFromAnyType()); + + // Matches construct expr that + // * has one argument + // * that argument satisfies ArgumentMatcher + // * argument is not the result of move constructor + // * parameter of constructor satisfies ParameterMatcher + // * constructed type is move constructible from argument + // ** or is value constructible from argument and argument is movable + // constructible + // ** constructed type has template constructor that can take by rref + // (like boost::any) + auto ConstructExprMatcher = + cxxConstructExpr( + hasType(qualType().bind("constructedType")), argumentCountIs(1), + unless(has(ignoringParenImpCasts( + cxxConstructExpr(ctorCallee(isMoveConstructor()))))), + hasArgument(0, hasType(hasCanonicalType(ignoringRefsAndConsts( + qualType().bind("argumentCanonicalType"))))), + ctorCallee(hasParameter(0, ParameterMatcher)), + hasArgument(0, ArgumentMatcher), + hasArgument(0, expr().bind("argument")), + hasType(qualType(anyOf(IsMoveOrCopyConstructibleFromParam, + IsCopyConstructibleFromParamViaTemplate)))) + .bind("construct"); + + auto IsCopyOrMoveConstructor = + anyOf(isCopyConstructor(), isMoveConstructor()); + + Finder->addMatcher( + returnStmt(has(ignoringParenImpCasts(cxxConstructExpr( + ctorCallee(IsCopyOrMoveConstructor), + has(ignoringParenImpCasts(ConstructExprMatcher)))))) + .bind("return"), + this); +} + +ReturnValueCopyCheck::ReturnValueCopyCheck(StringRef Name, + ClangTidyContext *Context) + : ClangTidyCheck(Name, Context), + IncludeStyle(utils::IncludeSorter::parseIncludeStyle( + Options.get("IncludeStyle", "llvm"))) {} + +void ReturnValueCopyCheck::registerPPCallbacks(CompilerInstance &Compiler) { + // Only register the preprocessor callbacks for C++; the functionality + // currently does not provide any benefit to other languages, despite being + // benign. + if (getLangOpts().CPlusPlus11) { + Inserter.reset(new utils::IncludeInserter( + Compiler.getSourceManager(), Compiler.getLangOpts(), IncludeStyle)); + Compiler.getPreprocessor().addPPCallbacks(Inserter->CreatePPCallbacks()); + } +} + +void ReturnValueCopyCheck::check(const MatchFinder::MatchResult &Result) { + const LangOptions &Opts = Result.Context->getLangOpts(); + + const auto *Argument = Result.Nodes.getNodeAs("argument"); + assert(Argument != nullptr); + + const auto *Type = Result.Nodes.getNodeAs("constructedType"); + assert(Type != nullptr); + assert(!Type->isNull()); + + std::string ReplacementText = Lexer::getSourceText( + CharSourceRange::getTokenRange(Argument->getSourceRange()), + *Result.SourceManager, Opts); + + ReplacementText = + Type->getAsString(getLangOpts()) + "(std::move(" + ReplacementText + "))"; + + auto Diag = diag(Argument->getExprLoc(), + "returned object is not moved; consider wrapping it with " + "std::move or changing return type to avoid the copy"); + Diag << FixItHint::CreateReplacement(Argument->getSourceRange(), + ReplacementText); + + if (auto IncludeFixit = Inserter->CreateIncludeInsertion( + Result.SourceManager->getFileID(Argument->getLocStart()), "utility", + /*IsAngled=*/true)) { + Diag << *IncludeFixit; + } +} + +} // namespace performance +} // namespace tidy +} // namespace clang Index: docs/ReleaseNotes.rst =================================================================== --- docs/ReleaseNotes.rst +++ docs/ReleaseNotes.rst @@ -232,6 +232,12 @@ Warns about range-based loop with a loop variable of const ref type where the type of the variable does not match the one returned by the iterator. +- New `performance-return-value-copy + `_ check + + Adds `std::move` in returns statements where returned object is copied and + adding `std::move` can make it being moved. + - New `performance-unnecessary-value-param `_ check Index: docs/clang-tidy/checks/list.rst =================================================================== --- docs/clang-tidy/checks/list.rst +++ docs/clang-tidy/checks/list.rst @@ -109,6 +109,7 @@ performance-faster-string-find performance-for-range-copy performance-implicit-cast-in-loop + performance-return-value-copy performance-unnecessary-copy-initialization performance-unnecessary-value-param readability-avoid-const-params-in-decls Index: docs/clang-tidy/checks/performance-return-value-copy.rst =================================================================== --- /dev/null +++ docs/clang-tidy/checks/performance-return-value-copy.rst @@ -0,0 +1,46 @@ +.. title:: clang-tidy - performance-return-value-copy + +performance-return-value-copy +========================== + +Adds `std::move` in returns statements where returned object is copied and +adding `std::move` can make it being moved. + +This check requires C++11 or higher to run. + +For returning object of type A in function with return type B +check triggers if: +- B is move constructible from A +- B is constructible from value of A and A is movable. + +check doesn't trigger when +- A is same type as B +- temporary object is returned +- returned object was declared as const or as reference +- A already has rvalue reference type + +For example: + +.. code-block:: c++ + + boost::optional get() { + string s; + ;;; + return s; + } + + // becomes + + boost::optional get() { + string s; + ;;; + return boost::optional(std::move(s)); + } + +Of course if we return an rvalue (e.g., temporary) we don’t have to move it: + +.. code-block:: c++ + + boost::optional get() { + return "str"; + } Index: test/clang-tidy/performance-return-value-copy.cpp =================================================================== --- /dev/null +++ test/clang-tidy/performance-return-value-copy.cpp @@ -0,0 +1,305 @@ +// RUN: %check_clang_tidy %s performance-return-value-copy %t +// CHECK-FIXES: {{^}}#include {{$}} + +// we need std::move mock +namespace std { +template +struct remove_reference { typedef _Tp type; }; + +template +struct remove_reference<_Tp &> { typedef _Tp type; }; + +template +struct remove_reference<_Tp &&> { typedef _Tp type; }; + +template +constexpr typename std::remove_reference<_Tp>::type && +move(_Tp &&__t) noexcept { return static_cast::type &&>(__t); } +} + +class SimpleClass { +public: + SimpleClass() = default; + SimpleClass(const SimpleClass &) = default; + SimpleClass(SimpleClass &&) = default; + + // We don't want to add std::move here because it will be added by compiler + SimpleClass foo(SimpleClass a, const SimpleClass b, SimpleClass &c, const SimpleClass &d, SimpleClass &&e, const SimpleClass &&f, char k) { + switch (k) { + case 'a': + return a; + case 'b': + return b; + case 'c': + return c; + case 'd': + return d; + case 'e': + return e; + case 'f': + return f; + default: + return SimpleClass(); + } + } +}; + +SimpleClass simpleClassFoo() { + return SimpleClass(); +} + +class FromValueClass { +public: + FromValueClass(SimpleClass a) {} + + FromValueClass foo(SimpleClass a, const SimpleClass b, SimpleClass &c, const SimpleClass &d, SimpleClass &&e, const SimpleClass &&f, char k) { + switch (k) { + case 'a': + // Because SimpleClass is move constructible + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: returned object is not moved; consider wrapping it with std::move or changing return type to avoid the copy [performance-return-value-copy] + // CHECK-FIXES: {{^ *}}return FromValueClass(std::move(a));{{$}} + case 'b': + return b; + case 'c': + return c; + case 'd': + return d; + case 'e': + return e; + case 'f': + return f; + case 'g': + return simpleClassFoo(); + default: + return SimpleClass(); + } + } +}; + +class FromRRefClass { +public: + FromRRefClass() = default; + FromRRefClass(const SimpleClass &a) {} + FromRRefClass(SimpleClass &&a) {} + + FromRRefClass foo1(SimpleClass a, const SimpleClass b, SimpleClass &c, const SimpleClass &d, SimpleClass &&e, const SimpleClass &&f, char k) { + switch (k) { + case 'a': + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: {{..}} + // CHECK-FIXES: {{^ *}}return FromRRefClass(std::move(a));{{$}} + + // We don't want to add std::move in cases 'b-f because + case 'b': + return b; + case 'c': + return c; + case 'd': + return d; + case 'e': + return e; + case 'f': + return f; + case 'g': + return simpleClassFoo(); + default: + return SimpleClass(); + } + } + + FromRRefClass foo2(char k) { + SimpleClass a; + const SimpleClass &b = a; + SimpleClass &c = a; + SimpleClass *d = &a; + const SimpleClass e; + + switch (k) { + case 'a': + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: {{..}} + // CHECK-FIXES: {{^ *}}return FromRRefClass(std::move(a));{{$}} + case 'b': + return b; + case 'c': + return c; + case 'd': + return *d; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: {{..}} + // CHECK-FIXES: {{^ *}}return FromRRefClass(std::move(*d));{{$}} + case 'e': + return e; + case 'f': + return simpleClassFoo(); + case 'x': + return SimpleClass(); + case 'y': + return FromRRefClass(SimpleClass()); + } + } + + FromRRefClass foo3(char k) { + SimpleClass a; + SimpleClass b; + FromRRefClass c; + switch (k) { + case 'a': + return std::move(a); + case 'b': + return FromRRefClass(std::move(a)); + case 'c': + return c; + default: + return FromRRefClass(); + } + } +}; + +template +FromRRefClass justTemplateFunction(T &&t) { + return t; +} + +void call_justTemplateFunction() { + justTemplateFunction(SimpleClass{}); + SimpleClass a; + justTemplateFunction(a); + justTemplateFunction(FromRRefClass{}); + FromRRefClass b; + justTemplateFunction(b); +} + +class FromValueWithDeleted { +public: + FromValueWithDeleted() = default; + FromValueWithDeleted(SimpleClass a) {} + FromValueWithDeleted(SimpleClass &&a) = delete; +}; + +FromValueWithDeleted foo9() { + SimpleClass a; + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: {{..}} + // CHECK-FIXES: {{^ *}}return FromValueWithDeleted(std::move(a));{{$}} +} + +template +struct old_optional { + old_optional(const T &t) {} +}; + +old_optional test_old_optional() { + SimpleClass a; + return a; +} + +template +struct optional { + optional(const T &) {} + optional(T &&) {} +}; + +optional test_optional() { + SimpleClass a; + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: {{..}} + // CHECK-FIXES: {{^ *}}return optional(std::move(a));{{$}} +} + +using Opt = optional; +Opt test_Opt() { + SimpleClass a; + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: {{..}} + // CHECK-FIXES: {{^ *}}return Opt(std::move(a));{{$}} +} + +struct any { + any() = default; + + template + any(T &&) {} + + any(any &&) = default; + any(const any &) = default; +}; + +any test_any() { + SimpleClass a; + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:10: warning: {{..}} + // CHECK-FIXES: {{^ *}}return any(std::move(a));{{$}} +} + +any test_any2() { + SimpleClass a; + return std::move(a); +} +any test_any3() { + SimpleClass a; + return any(std::move(a)); +} + +any test_any4() { + any a; + return a; +} + +class FromRefWithDeleted { +public: + FromRefWithDeleted(SimpleClass &&) = delete; + FromRefWithDeleted(const SimpleClass &a) {} +}; + +FromRefWithDeleted foo11(SimpleClass a) { + return a; +} + +class ClassWithUsings { +public: + using value = SimpleClass; + using const_value = const SimpleClass; + using lreference = SimpleClass &; + using const_lreference = const SimpleClass &; + using rreference = SimpleClass &&; + using const_rreference = const SimpleClass &&; + + ClassWithUsings(rreference) {} + ClassWithUsings(const_lreference) {} + + ClassWithUsings foo(value a, const_value b, lreference c, const_lreference d, rreference e, const_rreference f, char k) { + switch (k) { + case 'a': + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: {{..}} + // CHECK-FIXES: {{^ *}}return ClassWithUsings(std::move(a));{{$}} + case 'b': + return b; + case 'c': + return c; + case 'd': + return d; + case 'e': + return e; + case 'f': + return f; + case 'g': + return simpleClassFoo(); + default: + return SimpleClass(); + } + } +}; + +class FromRRefWithDefaultArgs { +public: + FromRRefWithDefaultArgs(SimpleClass &&, int k = 0) {} + FromRRefWithDefaultArgs(const SimpleClass &) {} + + FromRRefWithDefaultArgs foo(SimpleClass a) { + return a; + // CHECK-MESSAGES: :[[@LINE-1]]:12: warning: {{..}} + // CHECK-FIXES: {{^ *}}return FromRRefWithDefaultArgs(std::move(a));{{$}} + } +};