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 @@ -12,6 +12,7 @@ AvoidNonConstGlobalVariablesCheck.cpp AvoidReferenceCoroutineParametersCheck.cpp CppCoreGuidelinesTidyModule.cpp + ForwardingReferenceParamNotForwardedCheck.cpp InitVariablesCheck.cpp InterfacesGlobalInitCheck.cpp MacroUsageCheck.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 @@ -21,6 +21,7 @@ #include "AvoidGotoCheck.h" #include "AvoidNonConstGlobalVariablesCheck.h" #include "AvoidReferenceCoroutineParametersCheck.h" +#include "ForwardingReferenceParamNotForwardedCheck.h" #include "InitVariablesCheck.h" #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" @@ -70,6 +71,8 @@ "cppcoreguidelines-avoid-reference-coroutine-parameters"); CheckFactories.registerCheck( "cppcoreguidelines-explicit-virtual-functions"); + CheckFactories.registerCheck( + "cppcoreguidelines-forwarding-reference-param-not-forwarded"); CheckFactories.registerCheck( "cppcoreguidelines-init-variables"); CheckFactories.registerCheck( diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.h @@ -0,0 +1,36 @@ +//===--- ForwardingReferenceParamNotForwardedCheck.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_FORWARDINGREFERENCEPARAMNOTFORWARDEDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_FORWARDINGREFERENCEPARAMNOTFORWARDEDCHECK_H + +#include "../ClangTidyCheck.h" + +namespace clang::tidy::cppcoreguidelines { + +/// Warns when a function accepting a forwarding reference does anything besides +/// forwarding (with std::forward) the parameter in the body of the function. +/// This check implement CppCoreGuideline F.19. +/// +/// For the user-facing documentation see: +/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.html +class ForwardingReferenceParamNotForwardedCheck : public ClangTidyCheck { +public: + ForwardingReferenceParamNotForwardedCheck(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_FORWARDINGREFERENCEPARAMNOTFORWARDEDCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardingReferenceParamNotForwardedCheck.cpp @@ -0,0 +1,94 @@ +//===--- ForwardingReferenceParamNotForwardedCheck.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 "ForwardingReferenceParamNotForwardedCheck.h" +#include "clang/AST/ASTContext.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::cppcoreguidelines { + +namespace { + +AST_MATCHER_P(QualType, possiblyPackExpansionOf, + ast_matchers::internal::Matcher, InnerMatcher) { + return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder); +} + +AST_MATCHER(ParmVarDecl, isTemplateTypeOfFunction) { + ast_matchers::internal::Matcher Inner = possiblyPackExpansionOf( + qualType(rValueReferenceType(), + references(templateTypeParmType( + hasDeclaration(templateTypeParmDecl()))), + unless(references(qualType(isConstQualified()))))); + if (!Inner.matches(Node.getType(), Finder, Builder)) + return false; + + const auto *Function = dyn_cast(Node.getDeclContext()); + if (!Function) + return false; + + const FunctionTemplateDecl *FuncTemplate = + Function->getDescribedFunctionTemplate(); + if (!FuncTemplate) + return false; + + QualType ParamType = + Node.getType().getNonPackExpansionType()->getPointeeType(); + const auto *TemplateType = ParamType->getAs(); + if (!TemplateType) + return false; + + return TemplateType->getDepth() == + FuncTemplate->getTemplateParameters()->getDepth(); +} + +} // namespace + +void ForwardingReferenceParamNotForwardedCheck::registerMatchers( + MatchFinder *Finder) { + auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param"))); + + StatementMatcher ForwardCallMatcher = + callExpr( + callee(unresolvedLookupExpr(hasAnyDeclaration( + namedDecl(hasUnderlyingDecl(hasName("::std::forward")))))), + argumentCountIs(1), + hasArgument(0, declRefExpr(to(equalsBoundNode("param"))).bind("ref"))) + .bind("forward-call"); + + Finder->addMatcher( + parmVarDecl( + parmVarDecl().bind("param"), isTemplateTypeOfFunction(), + anyOf(hasAncestor(cxxConstructorDecl( + ToParam, isDefinition(), + optionally(hasDescendant(ForwardCallMatcher)))), + hasAncestor(functionDecl( + unless(cxxConstructorDecl()), ToParam, + hasBody(optionally(hasDescendant(ForwardCallMatcher))))))), + this); +} + +void ForwardingReferenceParamNotForwardedCheck::check( + const MatchFinder::MatchResult &Result) { + const auto *Param = Result.Nodes.getNodeAs("param"); + const auto *ForwardCall = Result.Nodes.getNodeAs("forward-call"); + + if (!Param) + return; + + if (!ForwardCall) { + diag(Param->getLocation(), + "forwarding reference parameter %0 is never forwarded " + "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 @@ -121,6 +121,11 @@ use-after-free errors and suggests avoiding captures or ensuring the lambda closure object has a guaranteed lifetime. +- New :doc:`cppcoreguidelines-forwarding-reference-param-not-forwarded + ` check. + + Warns when a forwarding reference parameter is not forwarded within the function body. + - New :doc:`cppcoreguidelines-rvalue-reference-param-not-moved ` check. diff --git a/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forwarding-reference-param-not-forwarded.rst @@ -0,0 +1,36 @@ +.. title:: clang-tidy - cppcoreguidelines-forwarding-reference-param-not-forwarded + +cppcoreguidelines-forwarding-reference-param-not-forwarded +========================================================== + +Warns when a forwarding reference parameter is not forwarded inside the +function body. + +Example: + +.. code-block:: c++ + + template + void wrapper(T&& t) { + impl(std::forward(t), 1, 2); // Correct + } + + template + void wrapper2(T&& t) { + impl(t, 1, 2); // Oops - should use std::forward(t) + } + + template + void wrapper3(T&& t) { + impl(std::move(t), 1, 2); // Also buggy - should use std::forward(t) + } + + template + void wrapper_function(F&& f) { + std::forward(f)(1, 2); // Correct + } + + template + void wrapper_function2(F&& f) { + f(1, 2); // Incorrect - may not invoke the desired qualified function operator + } 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 @@ -186,6 +186,7 @@ `cppcoreguidelines-avoid-goto `_, `cppcoreguidelines-avoid-non-const-global-variables `_, `cppcoreguidelines-avoid-reference-coroutine-parameters `_, + `cppcoreguidelines-forwarding-reference-param-not-forwarded `_, "Yes" `cppcoreguidelines-init-variables `_, "Yes" `cppcoreguidelines-interfaces-global-init `_, `cppcoreguidelines-macro-usage `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forwarding-reference-param-not-forwarded.cpp @@ -0,0 +1,105 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-forwarding-reference-param-not-forwarded %t + +// NOLINTBEGIN +namespace std { + +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; +template struct remove_reference { using type = T; }; + +template using remove_reference_t = typename remove_reference::type; + +template constexpr T &&forward(remove_reference_t &t) noexcept; +template constexpr T &&forward(remove_reference_t &&t) noexcept; +template constexpr remove_reference_t &&move(T &&x); + +} // namespace std +// NOLINTEND + +struct S { + S(); + S(const S&); + S(S&&) noexcept; + S& operator=(const S&); + S& operator=(S&&) noexcept; +}; + +template +void consumes_all(Ts&&...); + +namespace positive_cases { + +template +void does_not_forward(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:27: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t; +} + +template +void does_not_forward_invoked(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other = t(); +} + +template +void forwards_pairwise(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:28: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + auto first = std::forward(t.first); + auto second = std::forward(t.second); +} + +template +void does_not_forward_pack(Ts&&... ts) { + // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 'ts' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + consumes_all(ts...); +} + +template +class AClass { + template + void mixed_params(T&& t, U&& u) { + // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-forwarding-reference-param-not-forwarded] + T other1 = std::move(t); + U other2 = std::move(u); + } +}; + +} // namespace positive_cases + +namespace negative_cases { + +template +void does_forward(T&& t) { + T other = std::forward(t); +} + +template +void does_forward_pack(Ts&&... ts) { + consumes_all(std::forward(ts)...); +} + +void pass_by_value(S s) { + S other = std::move(s); +} + +void lvalue_ref(S& s) { + S other = std::move(s); +} + +void rvalue_ref(S&& s) { + S other = std::move(s); +} + +template +void templated_rvalue_ref(std::remove_reference_t&& t) { + T other = std::move(t); +} + +template +class AClass { + void rvalue_ref(T&& t) { + T other = std::move(t); + } +}; + +} // namespace negative_cases