diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt @@ -27,6 +27,7 @@ ProTypeStaticCastDowncastCheck.cpp ProTypeUnionAccessCheck.cpp ProTypeVarargCheck.cpp + RvalueReferenceParamNotMovedCheck.cpp SlicingCheck.cpp SpecialMemberFunctionsCheck.cpp VirtualClassDestructorCheck.cpp diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp @@ -36,6 +36,7 @@ #include "ProTypeStaticCastDowncastCheck.h" #include "ProTypeUnionAccessCheck.h" #include "ProTypeVarargCheck.h" +#include "RvalueReferenceParamNotMovedCheck.h" #include "SlicingCheck.h" #include "SpecialMemberFunctionsCheck.h" #include "VirtualClassDestructorCheck.h" @@ -98,6 +99,8 @@ "cppcoreguidelines-pro-type-union-access"); CheckFactories.registerCheck( "cppcoreguidelines-pro-type-vararg"); + CheckFactories.registerCheck( + "cppcoreguidelines-rvalue-reference-param-not-moved"); CheckFactories.registerCheck( "cppcoreguidelines-special-member-functions"); CheckFactories.registerCheck("cppcoreguidelines-slicing"); diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.h @@ -0,0 +1,34 @@ +//===--- RvalueReferenceParamNotMovedCheck.h - clang-tidy -------*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RVALUEREFERENCEPARAMNOTMOVEDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RVALUEREFERENCEPARAMNOTMOVEDCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::cppcoreguidelines { + +/// Warns when an rvalue reference function parameter is never moved within +/// the function body. This check implements CppCoreGuideline F.18. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.html +class RvalueReferenceParamNotMovedCheck : public ClangTidyCheck { +public: + RvalueReferenceParamNotMovedCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; + bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { + return LangOpts.CPlusPlus11; + } +}; + +} // namespace clang::tidy::cppcoreguidelines + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_RVALUEREFERENCEPARAMNOTMOVEDCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/RvalueReferenceParamNotMovedCheck.cpp @@ -0,0 +1,153 @@ +//===--- RvalueReferenceParamNotMovedCheck.cpp - clang-tidy ---------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "RvalueReferenceParamNotMovedCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::cppcoreguidelines { + +void RvalueReferenceParamNotMovedCheck::registerMatchers(MatchFinder *Finder) { + auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param"))); + + internal::Matcher ReferenceTypeOrAnything = + anyOf(qualType(references(templateTypeParmType(hasDeclaration( + templateTypeParmDecl().bind("template-type")))), + unless(references(qualType(isConstQualified())))), + anything()); + + Finder->addMatcher( + parmVarDecl(allOf( + parmVarDecl(hasType(type(rValueReferenceType()))).bind("param"), + parmVarDecl( + equalsBoundNode("param"), + unless( + hasType(qualType(references(substTemplateTypeParmType())))), + hasType(ReferenceTypeOrAnything), + anyOf(hasAncestor(compoundStmt(hasParent( + lambdaExpr(has(cxxRecordDecl(has(cxxMethodDecl( + ToParam, hasName("operator()")))))) + .bind("containing-lambda")))), + hasAncestor(cxxConstructorDecl(isDefinition(), ToParam, + unless(isMoveConstructor()), + isDefinition(), ToParam) + .bind("containing-ctor")), + hasAncestor( + functionDecl( + isDefinition(), ToParam, + unless(cxxConstructorDecl(isMoveConstructor())), + unless(cxxMethodDecl(isMoveAssignmentOperator()))) + .bind("containing-func")))))), + this); +} + +static bool isValueCapturedByAnyLambda(ASTContext &Context, + const FunctionDecl *Function, + const DeclRefExpr *MoveTarget, + const ParmVarDecl *Param) { + SmallVector Matches = + match(lambdaExpr(hasAncestor(equalsNode(Function)), + hasDescendant(declRefExpr(equalsNode(MoveTarget)))) + .bind("lambda"), + Context); + for (const BoundNodes &Match : Matches) { + const auto *Lambda = Match.getNodeAs("lambda"); + bool ParamIsValueCaptured = + std::find_if(Lambda->capture_begin(), Lambda->capture_end(), + [&](const LambdaCapture &Capture) { + return Capture.capturesVariable() && + Capture.getCapturedVar() == Param && + Capture.getCaptureKind() == LCK_ByCopy; + }) != Lambda->capture_end(); + if (ParamIsValueCaptured) { + return true; + } + } + return false; +} + +void RvalueReferenceParamNotMovedCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Param = Result.Nodes.getNodeAs("param"); + const auto *ContainingLambda = + Result.Nodes.getNodeAs("containing-lambda"); + const auto *ContainingCtor = + Result.Nodes.getNodeAs("containing-ctor"); + const auto *ContainingFunc = + Result.Nodes.getNodeAs("containing-func"); + const auto *TemplateType = + Result.Nodes.getNodeAs("template-type"); + + if (!Param) + return; + + const auto *Function = dyn_cast(Param->getDeclContext()); + if (!Function) + return; + + if (TemplateType) { + if (const FunctionTemplateDecl *FuncTemplate = + Function->getDescribedFunctionTemplate()) { + const TemplateParameterList *Params = + FuncTemplate->getTemplateParameters(); + if (llvm::is_contained(*Params, TemplateType)) { + // Ignore forwarding reference + return; + } + } + } + + StatementMatcher MoveCallMatcher = + callExpr(anyOf(callee(functionDecl(hasName("::std::move"))), + callee(unresolvedLookupExpr(hasAnyDeclaration(namedDecl( + hasUnderlyingDecl(hasName("::std::move"))))))), + argumentCountIs(1), + hasArgument(0, declRefExpr(to(equalsNode(Param))).bind("ref"))); + + SmallVector Matches; + if (ContainingLambda) { + if (!ContainingLambda->getBody()) + return; + Matches = match(findAll(MoveCallMatcher), *ContainingLambda->getBody(), + *Result.Context); + } else if (ContainingCtor) { + Matches = match(findAll(cxxConstructorDecl(hasDescendant(MoveCallMatcher))), + *ContainingCtor, *Result.Context); + } else if (ContainingFunc) { + if (!ContainingFunc->getBody()) + return; + Matches = match(findAll(MoveCallMatcher), *ContainingFunc->getBody(), + *Result.Context); + } else { + return; + } + + int MoveExprsCount = Matches.size(); + for (const BoundNodes &Match : Matches) { + // The DeclRefExprs of non-initializer value captured variables refer to + // the original variable declaration in the AST. In such cases, we exclude + // those DeclRefExprs since they are not actually moving the original + // variable. + if (isValueCapturedByAnyLambda(*Result.Context, Function, + Match.getNodeAs("ref"), + Param)) { + --MoveExprsCount; + } + } + + if (MoveExprsCount == 0) { + diag(Param->getLocation(), + "rvalue reference parameter %0 is never moved from " + "inside the function body") + << Param; + } +} + +} // namespace clang::tidy::cppcoreguidelines diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -97,6 +97,12 @@ New checks ^^^^^^^^^^ +- New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved + ` check. + + Warns when an rvalue reference function parameter is never moved within + the function body. + New check aliases ^^^^^^^^^^^^^^^^^ diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/rvalue-reference-param-not-moved.rst @@ -0,0 +1,22 @@ +.. title:: clang-tidy - cppcoreguidelines-rvalue-reference-param-not-moved + +cppcoreguidelines-rvalue-reference-param-not-moved +================================================== + +Warns when an rvalue reference function parameter is never moved within +the function body. + +Rvalue reference parameters indicate a parameter that should be moved with +``std::move`` from within the function body. Any such parameter that is +never moved is confusing and potentially indicative of a buggy program. + +Examples: + +.. code-block:: c++ + + void logic(std::string&& Input) { + std::string Copy(Input); // Oops - forgot to std::move + } + +This check implements +`CppCoreGuideline F.18 `_. diff --git a/clang-tools-extra/docs/clang-tidy/checks/list.rst b/clang-tools-extra/docs/clang-tidy/checks/list.rst --- a/clang-tools-extra/docs/clang-tidy/checks/list.rst +++ b/clang-tools-extra/docs/clang-tidy/checks/list.rst @@ -200,6 +200,7 @@ `cppcoreguidelines-pro-type-static-cast-downcast `_, "Yes" `cppcoreguidelines-pro-type-union-access `_, `cppcoreguidelines-pro-type-vararg `_, + `cppcoreguidelines-rvalue-reference-param-not-moved `_, `cppcoreguidelines-slicing `_, `cppcoreguidelines-special-member-functions `_, `cppcoreguidelines-virtual-class-destructor `_, "Yes" diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/rvalue-reference-param-not-moved.cpp @@ -0,0 +1,295 @@ +// RUN: %check_clang_tidy -std=c++11 %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- -- -fno-delayed-template-parsing +// RUN: %check_clang_tidy -check-suffix=,CXX14 -std=c++14-or-later %s cppcoreguidelines-rvalue-reference-param-not-moved %t -- -- -fno-delayed-template-parsing + +// NOLINTBEGIN +namespace std { +template +struct remove_reference; + +template +struct remove_reference { + typedef _Tp type; +}; + +template +constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept; + +template +constexpr _Tp && +forward(typename remove_reference<_Tp>::type &__t) noexcept; + +} +// NOLINTEND + +struct Obj { + Obj(); + Obj(const Obj&); + Obj& operator=(const Obj&); + Obj(Obj&&); + Obj& operator=(Obj&&); + void member() const; +}; + +void consumes_object(Obj); + +void never_moves_param(Obj&& o) { + // CHECK-MESSAGES: :[[@LINE-1]]:30: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + o.member(); +} + +void copies_object(Obj&& o) { + // CHECK-MESSAGES: :[[@LINE-1]]:26: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + Obj copy = o; +} + +template +void never_moves_param_template(Obj&& o, T t) { + // CHECK-MESSAGES: :[[@LINE-1]]:39: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + o.member(); +} + +void never_moves_params(Obj&& o1, Obj&& o2) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: rvalue reference parameter 'o1' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + // CHECK-MESSAGES: :[[@LINE-2]]:41: warning: rvalue reference parameter 'o2' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] +} + +void never_moves_some_params(Obj&& o1, Obj&& o2) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: rvalue reference parameter 'o1' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + + Obj other{std::move(o2)}; +} + +void never_moves_mixed(Obj o1, Obj&& o2) { + // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: rvalue reference parameter 'o2' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] +} + +void lambda_captures_parameter_as_value(Obj&& o) { + auto f = [o]() { + consumes_object(std::move(o)); + }; + // CHECK-MESSAGES: :[[@LINE-4]]:47: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] +} + +void lambda_captures_parameter_as_value_nested(Obj&& o) { + // CHECK-MESSAGES: :[[@LINE-1]]:54: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + auto f = [&o]() { + auto f_nested = [o]() { + consumes_object(std::move(o)); + }; + }; + auto f2 = [o]() { + auto f_nested = [&o]() { + consumes_object(std::move(o)); + }; + }; + auto f3 = [o]() { + auto f_nested = [&o]() { + auto f_nested_inner = [&o]() { + consumes_object(std::move(o)); + }; + }; + }; + auto f4 = [&o]() { + auto f_nested = [&o]() { + auto f_nested_inner = [o]() { + consumes_object(std::move(o)); + }; + }; + }; +} + +void misc_lambda_checks() { + auto never_moves = [](Obj&& o1) { + Obj other{o1}; + }; + // CHECK-MESSAGES: :[[@LINE-3]]:31: warning: rvalue reference parameter 'o1' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + +#if __cplusplus >= 201402L + auto never_moves_with_auto_param = [](Obj&& o1, auto& v) { + Obj other{o1}; + }; + // CHECK-MESSAGES-CXX14: :[[@LINE-3]]:47: warning: rvalue reference parameter 'o1' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] +#endif +} + +template +void forwarding_ref(T&& t) { + t.member(); +} + +template +void forwarding_ref_forwarded(T&& t) { + forwarding_ref(std::forward(t)); +} + +template +void type_pack(Ts&&... ts) { + (forwarding_ref(std::forward(ts)), ...); +} + +void call_forwarding_functions() { + Obj o; + + forwarding_ref(Obj{}); + type_pack(Obj{}); + type_pack(Obj{}, o); + type_pack(Obj{}, Obj{}); +} + +void moves_parameter(Obj&& o) { + Obj moved = std::move(o); +} + +void moves_parameter_extra_parens(Obj&& o) { + Obj moved = std::move((o)); +} + +template +struct mypair { + T1 first; + T2 second; +}; + +void moves_member_of_parameter(mypair&& pair) { + // CHECK-MESSAGES: :[[@LINE-1]]:51: warning: rvalue reference parameter 'pair' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + Obj a = std::move(pair.first); + Obj b = std::move(pair.second); +} + +template +struct myoptional { + T& operator*() &; + T&& operator*() &&; +}; + +void moves_deref_optional(myoptional&& opt) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: rvalue reference parameter 'opt' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + Obj other = std::move(*opt); +} + +void moves_optional_then_deref_resulting_rvalue(myoptional&& opt) { + Obj other = *std::move(opt); +} + +void pass_by_lvalue_reference(Obj& o) { + o.member(); +} + +void pass_by_value(Obj o) { + o.member(); +} + +void pass_by_const_lvalue_reference(const Obj& o) { + o.member(); +} + +void lambda_captures_parameter_as_reference(Obj&& o) { + auto f = [&o]() { + consumes_object(std::move(o)); + }; +} + +void lambda_captures_parameter_as_reference_nested(Obj&& o) { + auto f = [&o]() { + auto f_nested = [&o]() { + auto f_nested2 = [&o]() { + consumes_object(std::move(o)); + }; + }; + }; +} + +#if __cplusplus >= 201402L +void lambda_captures_parameter_generalized(Obj&& o) { + auto f = [o = std::move(o)]() { + consumes_object(std::move(o)); + }; +} +#endif + +void negative_lambda_checks() { + auto never_moves_nested = [](Obj&& o1) { + auto nested = [&]() { + Obj other{std::move(o1)}; + }; + }; + +#if __cplusplus >= 201402L + auto auto_lvalue_ref_param = [](auto& o1) { + Obj other{o1}; + }; + + auto auto_forwarding_ref_param = [](auto&& o1) { + Obj other{o1}; + }; + + auto does_move_auto_rvalue_ref_param = [](auto&& o1) { + Obj other{std::forward(o1)}; + }; +#endif + + auto does_move = [](Obj&& o1) { + Obj other{std::move(o1)}; + }; + + auto not_rvalue_ref = [](Obj& o1) { + Obj other{std::move(o1)}; + }; + + Obj local; + auto captures = [local]() { }; +} + +struct AClass { + void member_with_lambda_no_move(Obj&& o) { + // CHECK-MESSAGES: :[[@LINE-1]]:41: warning: rvalue reference parameter 'o' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] + auto captures_this = [=, this]() { + Obj other = std::move(o); + }; + } + void member_with_lambda_that_moves(Obj&& o) { + auto captures_this = [&, this]() { + Obj other = std::move(o); + }; + } +}; + +void useless_move(Obj&& o) { + // FIXME - The object is not actually moved from - this should probably be + // flagged by *some* check. Which one? + std::move(o); +} + +template +class TemplatedClass; + +template +void unresolved_lookup(TemplatedClass&& o) { + TemplatedClass moved = std::move(o); +} + +struct DefinesMove { + DefinesMove(DefinesMove&& rhs) : o(std::move(rhs.o)) { } + DefinesMove& operator=(DefinesMove&& rhs) { + if (this != &rhs) { + o = std::move(rhs.o); + } + return *this; + } + Obj o; +}; + +struct AnotherObj { + AnotherObj(Obj&& o) : o(std::move(o)) {} + AnotherObj(Obj&& o, int) { o = std::move(o); } + Obj o; +}; + +template +struct AClassTemplate { + void moves(T&& t) { + T other = std::move(t); + } + void never_moves(T&& t) {} + // CHECK-MESSAGES: :[[@LINE-1]]:24: warning: rvalue reference parameter 't' is never moved from inside the function body [cppcoreguidelines-rvalue-reference-param-not-moved] +};