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 @@ -15,6 +15,7 @@ InterfacesGlobalInitCheck.cpp MacroUsageCheck.cpp MisleadingCaptureDefaultByValueCheck.cpp + MissingStdForwardCheck.cpp NarrowingConversionsCheck.cpp NoMallocCheck.cpp OwningMemoryCheck.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 @@ -25,6 +25,7 @@ #include "InterfacesGlobalInitCheck.h" #include "MacroUsageCheck.h" #include "MisleadingCaptureDefaultByValueCheck.h" +#include "MissingStdForwardCheck.h" #include "NarrowingConversionsCheck.h" #include "NoMallocCheck.h" #include "OwningMemoryCheck.h" @@ -77,6 +78,8 @@ "cppcoreguidelines-macro-usage"); CheckFactories.registerCheck( "cppcoreguidelines-misleading-capture-default-by-value"); + CheckFactories.registerCheck( + "cppcoreguidelines-missing-std-forward"); CheckFactories.registerCheck( "cppcoreguidelines-narrowing-conversions"); CheckFactories.registerCheck("cppcoreguidelines-no-malloc"); diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.h @@ -0,0 +1,38 @@ +//===--- MissingStdForwardCheck.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_MISSINGSTDFORWARDCHECK_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MISSINGSTDFORWARDCHECK_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/missing-std-forward.html +class MissingStdForwardCheck : public ClangTidyCheck { +public: + MissingStdForwardCheck(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; + } + std::optional getCheckTraversalKind() const override { + return TK_IgnoreUnlessSpelledInSource; + } +}; + +} // namespace clang::tidy::cppcoreguidelines + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_MISSINGSTDFORWARDCHECK_H diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp @@ -0,0 +1,90 @@ +//===--- MissingStdForwardCheck.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 "MissingStdForwardCheck.h" +#include "../utils/Matchers.h" +#include "clang/AST/ASTContext.h" +#include "clang/AST/ExprConcepts.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" + +using namespace clang::ast_matchers; + +namespace clang::tidy::cppcoreguidelines { + +namespace { + +using matchers::hasUnevaluatedContext; + +AST_MATCHER_P(QualType, possiblyPackExpansionOf, + ast_matchers::internal::Matcher, InnerMatcher) { + return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder); +} + +AST_MATCHER(ParmVarDecl, isTemplateTypeParameter) { + 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 MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) { + auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param"))); + + auto ForwardCallMatcher = callExpr( + forCallable(equalsBoundNode("func")), argumentCountIs(1), + callee(unresolvedLookupExpr(hasAnyDeclaration( + namedDecl(hasUnderlyingDecl(hasName("::std::forward")))))), + hasArgument(0, declRefExpr(to(equalsBoundNode("param"))).bind("ref")), + unless(anyOf(hasAncestor(typeLoc()), + hasAncestor(expr(hasUnevaluatedContext()))))); + + Finder->addMatcher( + parmVarDecl(parmVarDecl().bind("param"), isTemplateTypeParameter(), + hasAncestor(functionDecl().bind("func")), + hasAncestor(functionDecl( + isDefinition(), equalsBoundNode("func"), ToParam, + unless(hasDescendant(std::move(ForwardCallMatcher)))))), + this); +} + +void MissingStdForwardCheck::check(const MatchFinder::MatchResult &Result) { + const auto *Param = Result.Nodes.getNodeAs("param"); + + if (!Param) + return; + + 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 @@ -138,6 +138,12 @@ Warns when lambda specify a by-value capture default and capture ``this``. +- New :doc:`cppcoreguidelines-missing-std-forward + ` 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/missing-std-forward.rst b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst new file mode 100644 --- /dev/null +++ b/clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/missing-std-forward.rst @@ -0,0 +1,39 @@ +.. title:: clang-tidy - cppcoreguidelines-missing-std-forward + +cppcoreguidelines-missing-std-forward +===================================== + +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 + } + +This check implements +`CppCoreGuideline F.19 `_. 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 @@ -191,6 +191,7 @@ `cppcoreguidelines-interfaces-global-init `_, `cppcoreguidelines-macro-usage `_, `cppcoreguidelines-misleading-capture-default-by-value `_, "Yes" + `cppcoreguidelines-missing-std-forward `_, `cppcoreguidelines-narrowing-conversions `_, `cppcoreguidelines-no-malloc `_, `cppcoreguidelines-owning-memory `_, diff --git a/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp @@ -0,0 +1,150 @@ +// RUN: %check_clang_tidy %s cppcoreguidelines-missing-std-forward %t -- -- -fno-delayed-template-parsing + +// 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-missing-std-forward] + 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-missing-std-forward] + 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-missing-std-forward] + 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-missing-std-forward] + consumes_all(ts...); +} + +template +class AClass { + + template + AClass(U&& u) : data(u) {} + // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + template + AClass& operator=(U&& u) { } + // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: forwarding reference parameter 'u' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + + 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-missing-std-forward] + T other1 = std::move(t); + U other2 = std::move(u); + } + + T data; +}; + +template +void does_not_forward_in_evaluated_code(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:45: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + using result_t = decltype(std::forward(t)); + unsigned len = sizeof(std::forward(t)); + T other = t; +} + +template +void lambda_value_capture(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=]() { T other = std::forward(t); }; +} + +template +void lambda_value_reference(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&]() { T other = std::forward(t); }; +} + +} // namespace positive_cases + +namespace negative_cases { + +template +void just_a_decl(T&&t); + +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 { + + template + AClass(U&& u) : data(std::forward(u)) {} + + template + AClass& operator=(U&& u) { + data = std::forward(u); + } + + void rvalue_ref(T&& t) { + T other = std::move(t); + } + + T data; +}; + +} // namespace negative_cases