ccotter created this revision.
Herald added subscribers: PiotrZSL, carlosgalvezp, kbarton, xazax.hun, nemanjai.
Herald added a reviewer: njames93.
Herald added a project: All.
ccotter requested review of this revision.
Herald added a project: clang-tools-extra.
Herald added a subscriber: cfe-commits.
Implement the std::forward specific enforcement of ES.56.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D146888
Files:
clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp
clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h
clang-tools-extra/docs/ReleaseNotes.rst
clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst
clang-tools-extra/docs/clang-tidy/checks/list.rst
clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp
Index: clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/forward-non-forwarding-parameter.cpp
@@ -0,0 +1,163 @@
+// RUN: %check_clang_tidy %s cppcoreguidelines-forward-non-forwarding-parameter %t
+
+// NOLINTBEGIN
+namespace std {
+
+template <typename T> struct remove_reference { using type = T; };
+template <typename T> struct remove_reference<T&> { using type = T; };
+template <typename T> struct remove_reference<T&&> { using type = T; };
+
+template <typename T> using remove_reference_t = typename remove_reference<T>::type;
+
+template <typename T> constexpr T &&forward(remove_reference_t<T> &t) noexcept;
+template <typename T> constexpr T &&forward(remove_reference_t<T> &&t) noexcept;
+
+} // namespace std
+// NOLINTEND
+
+struct Obj {
+ Obj();
+ Obj(const Obj&);
+ Obj(Obj&&) noexcept;
+ Obj& operator=(const Obj&);
+ Obj& operator=(Obj&&) noexcept;
+};
+
+template <class... Ts>
+void consumes_all(Ts&&...);
+
+namespace positive_cases {
+
+void forward_local_object() {
+ Obj obj;
+ Obj& obj_ref = obj;
+
+ Obj obj2 = std::forward<Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+
+ Obj obj3 = std::forward<Obj>(obj_ref);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj_ref' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_value_param(Obj obj) {
+ Obj obj2 = std::forward<Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class... Ts>
+void forward_pack_of_values(Ts... ts) {
+ consumes_all(std::forward<Ts>(ts)...);
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: calling std::forward on non-forwarding reference 'ts' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class... Ts>
+void forward_pack_of_lvalue_refs(Ts&... ts) {
+ consumes_all(std::forward<Ts>(ts)...);
+ // CHECK-MESSAGES: :[[@LINE-1]]:33: warning: calling std::forward on non-forwarding reference 'ts' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_lvalue_ref(Obj& obj) {
+ Obj obj2 = std::forward<Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_const_lvalue_ref(const Obj& obj) {
+ Obj obj2 = std::forward<const Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+void forward_rvalue_ref(Obj&& obj) {
+ Obj obj2 = std::forward<Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:32: warning: calling std::forward on rvalue reference 'obj'; use std::move instead [cppcoreguidelines-forward-non-forwarding-parameter]
+ // CHECK-FIXES: Obj obj2 = std::move(obj);
+}
+
+void forward_const_rvalue_ref(const Obj&& obj) {
+ Obj obj2 = std::forward<const Obj>(obj);
+ // CHECK-MESSAGES: :[[@LINE-1]]:38: warning: calling std::forward on non-forwarding reference 'obj' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_value_t(T t) {
+ T other = std::forward<T>(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_lvalue_ref_t(T& t) {
+ T other = std::forward<T>(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:29: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+void forward_const_rvalue_ref_t(const T&& t) {
+ T other = std::forward<const T>(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:35: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+}
+
+template <class T>
+struct SomeClass
+{
+ void forwards_lvalue_ref(T& t) {
+ T other = std::forward<T>(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: calling std::forward on non-forwarding reference 't' [cppcoreguidelines-forward-non-forwarding-parameter]
+ }
+
+ void forwards_rvalue_ref(T&& t) {
+ T other = std::forward<T>(t);
+ // CHECK-MESSAGES: :[[@LINE-1]]:31: warning: calling std::forward on rvalue reference 't'; use std::move instead [cppcoreguidelines-forward-non-forwarding-parameter]
+ // CHECK-FIXES: T other = std::move(t);
+ }
+};
+
+} // namespace positive_cases
+
+namespace negative_cases {
+
+template <class T>
+void forwards_param(T&& t) {
+ T other = std::forward<T>(t);
+}
+
+template <class... Ts>
+void forwards_param_pack(Ts&&... ts) {
+ consumes_all(std::forward<Ts>(ts)...);
+}
+
+template <class A>
+struct SomeClass {
+ template <class T>
+ void forwards_param(T&& t) {
+ T other = std::forward<T>(t);
+ }
+
+ template <class T, class U>
+ void forwards_param(T&& t) {
+ T other = std::forward<T>(t);
+ }
+
+ template <class... Ts>
+ void forwards_param_pack(Ts&&... ts) {
+ consumes_all(1, std::forward<Ts>(ts)...);
+ }
+};
+
+void instantiates_calls() {
+ Obj o;
+ forwards_param(0);
+ forwards_param(Obj{});
+ forwards_param(o);
+
+ forwards_param_pack(0);
+ forwards_param_pack(Obj{});
+ forwards_param_pack(o);
+ forwards_param_pack(0, Obj{}, o);
+
+ SomeClass<Obj> someObj;
+ someObj.forwards_param(0);
+ someObj.forwards_param(Obj{});
+ someObj.forwards_param(o);
+ someObj.forwards_param_pack(0, Obj{}, o);
+}
+
+} // namespace negative_cases
Index: clang-tools-extra/docs/clang-tidy/checks/list.rst
===================================================================
--- clang-tools-extra/docs/clang-tidy/checks/list.rst
+++ clang-tools-extra/docs/clang-tidy/checks/list.rst
@@ -186,6 +186,7 @@
`cppcoreguidelines-avoid-goto <cppcoreguidelines/avoid-goto.html>`_,
`cppcoreguidelines-avoid-non-const-global-variables <cppcoreguidelines/avoid-non-const-global-variables.html>`_,
`cppcoreguidelines-avoid-reference-coroutine-parameters <cppcoreguidelines/avoid-reference-coroutine-parameters.html>`_,
+ `cppcoreguidelines-forward-non-forwarding-parameter <cppcoreguidelines/forward-non-forwarding-parameter.html>`_, "Yes"
`cppcoreguidelines-init-variables <cppcoreguidelines/init-variables.html>`_, "Yes"
`cppcoreguidelines-interfaces-global-init <cppcoreguidelines/interfaces-global-init.html>`_,
`cppcoreguidelines-macro-usage <cppcoreguidelines/macro-usage.html>`_,
Index: clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst
===================================================================
--- /dev/null
+++ clang-tools-extra/docs/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.rst
@@ -0,0 +1,35 @@
+.. title:: clang-tidy - cppcoreguidelines-forward-non-forwarding-parameter
+
+cppcoreguidelines-forward-non-forwarding-parameter
+==================================================
+
+Warns when ``std::forward`` is used on a non-forwarding reference.
+``std::forward`` is a named cast that preserves the value category of
+the input parameter. It is used in templated functions when the
+parameter should be "forwarded" to another function or constructor
+within the function template body. Using ``std::forward`` on
+non-forwarding references can lead to unexpected or buggy behavior.
+
+Example:
+
+.. code-block:: c++
+
+ void by_lvalue_ref(Object& Input) {
+ // This will actually move Input, always. This is confusing
+ // since the caller of by_lvalue_ref did not std::move, and
+ // could lead to use after move bugs.
+
+ // If a copy was intended, remove std::forward.
+ // If a move is intended, change Input to 'Object&&' or 'Object'
+ // and update the caller.
+ If that is what is intended, use std::move.
+ Object Copy(std::forward<Object>(Input));
+ }
+
+ void move_param(Object&& Input) {
+ Object Copy(std::move(Input)); // Ok
+ Object Copy2(std::forward<Object>(Input)); // Use std::move instead
+ }
+
+This check implements the ``std::forward`` specific enforcements of
+`CppCoreGuideline ES.56 <http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es56-write-stdmove-only-when-you-need-to-explicitly-move-an-object-to-another-scope>
Index: clang-tools-extra/docs/ReleaseNotes.rst
===================================================================
--- clang-tools-extra/docs/ReleaseNotes.rst
+++ clang-tools-extra/docs/ReleaseNotes.rst
@@ -133,6 +133,11 @@
Checks that all implicit and explicit inline functions in header files are
tagged with the ``LIBC_INLINE`` macro.
+- New :doc:`cppcoreguidelines-forward-non-forwarding-parameter
+ <clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter>` check.
+
+ Warns when ``std::forward`` is used on a non-forwarding reference.
+
New check aliases
^^^^^^^^^^^^^^^^^
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.h
@@ -0,0 +1,32 @@
+//===--- ForwardNonForwardingParameterCheck.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_FORWARDNONFORWARDINGPARAMETERCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_FORWARDNONFORWARDINGPARAMETERCHECK_H
+
+#include "../ClangTidyCheck.h"
+
+namespace clang::tidy::cppcoreguidelines {
+
+/// Warns when std::forward is called on a non-forwarding reference. This
+/// check implements the std::forward specific enforcement section of
+/// CppCoreGuideline ES.56.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/cppcoreguidelines/forward-non-forwarding-parameter.html
+class ForwardNonForwardingParameterCheck : public ClangTidyCheck {
+public:
+ ForwardNonForwardingParameterCheck(StringRef Name, ClangTidyContext *Context)
+ : ClangTidyCheck(Name, Context) {}
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+
+} // namespace clang::tidy::cppcoreguidelines
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CPPCOREGUIDELINES_FORWARDNONFORWARDINGPARAMETERCHECK_H
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp
===================================================================
--- /dev/null
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/ForwardNonForwardingParameterCheck.cpp
@@ -0,0 +1,115 @@
+//===--- ForwardNonForwardingParameterCheck.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 "ForwardNonForwardingParameterCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::cppcoreguidelines {
+
+namespace {
+
+AST_MATCHER_P(QualType, possiblyPackExpansionOf,
+ ast_matchers::internal::Matcher<QualType>, InnerMatcher) {
+ return InnerMatcher.matches(Node.getNonPackExpansionType(), Finder, Builder);
+}
+
+AST_MATCHER(ParmVarDecl, isForwardingRefTypeOfFunction) {
+ ast_matchers::internal::Matcher<QualType> 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<FunctionDecl>(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<TemplateTypeParmType>();
+ if (!TemplateType)
+ return false;
+
+ return TemplateType->getDepth() ==
+ FuncTemplate->getTemplateParameters()->getDepth();
+}
+
+AST_MATCHER(ParmVarDecl, isInInstantiation) {
+ const auto *Function = dyn_cast<FunctionDecl>(Node.getDeclContext());
+ if (!Function)
+ return false;
+ return Function->isTemplateInstantiation();
+}
+
+} // namespace
+
+void ForwardNonForwardingParameterCheck::registerMatchers(MatchFinder *Finder) {
+ auto RvalueRefDecl =
+ varDecl(hasType(type(rValueReferenceType())),
+ unless(hasType(references(qualType(isConstQualified())))));
+ auto VarMatcher =
+ varDecl(unless(parmVarDecl(isForwardingRefTypeOfFunction())),
+ unless(parmVarDecl(isInInstantiation())),
+ optionally(RvalueRefDecl.bind("rvalue-var")))
+ .bind("var");
+
+ Finder->addMatcher(
+ callExpr(anyOf(callee(functionDecl(hasName("::std::forward"))),
+ callee(unresolvedLookupExpr(hasAnyDeclaration(namedDecl(
+ hasUnderlyingDecl(hasName("::std::forward"))))))),
+ argumentCountIs(1),
+ hasArgument(0, declRefExpr(to(VarMatcher)).bind("ref")))
+ .bind("call"),
+ this);
+}
+
+void ForwardNonForwardingParameterCheck::check(
+ const MatchFinder::MatchResult &Result) {
+ const auto *Call = Result.Nodes.getNodeAs<CallExpr>("call");
+ const auto *Ref = Result.Nodes.getNodeAs<DeclRefExpr>("ref");
+ const auto *Var = Result.Nodes.getNodeAs<VarDecl>("var");
+ const auto *RvalueRefVar = Result.Nodes.getNodeAs<VarDecl>("rvalue-var");
+
+ if (!Call || !Var || !Ref)
+ return;
+
+ if (RvalueRefVar) {
+ ASTContext &Context = *Result.Context;
+
+ std::string MoveText = "std::move(";
+ MoveText += Lexer::getSourceText(
+ CharSourceRange::getTokenRange(Ref->getBeginLoc(), Ref->getEndLoc()),
+ Context.getSourceManager(), Context.getLangOpts());
+ MoveText += ")";
+
+ diag(Ref->getLocation(),
+ "calling std::forward on rvalue reference %0; use std::move instead")
+ << Var
+ << FixItHint::CreateReplacement(
+ CharSourceRange::getTokenRange(Call->getBeginLoc(),
+ Call->getEndLoc()),
+ MoveText);
+ } else {
+ diag(Ref->getLocation(),
+ "calling std::forward on non-forwarding reference %0")
+ << Var;
+ }
+}
+
+} // namespace clang::tidy::cppcoreguidelines
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CppCoreGuidelinesTidyModule.cpp
@@ -21,6 +21,7 @@
#include "AvoidGotoCheck.h"
#include "AvoidNonConstGlobalVariablesCheck.h"
#include "AvoidReferenceCoroutineParametersCheck.h"
+#include "ForwardNonForwardingParameterCheck.h"
#include "InitVariablesCheck.h"
#include "InterfacesGlobalInitCheck.h"
#include "MacroUsageCheck.h"
@@ -70,6 +71,8 @@
"cppcoreguidelines-avoid-reference-coroutine-parameters");
CheckFactories.registerCheck<modernize::UseOverrideCheck>(
"cppcoreguidelines-explicit-virtual-functions");
+ CheckFactories.registerCheck<ForwardNonForwardingParameterCheck>(
+ "cppcoreguidelines-forward-non-forwarding-parameter");
CheckFactories.registerCheck<InitVariablesCheck>(
"cppcoreguidelines-init-variables");
CheckFactories.registerCheck<InterfacesGlobalInitCheck>(
Index: clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
===================================================================
--- clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
+++ clang-tools-extra/clang-tidy/cppcoreguidelines/CMakeLists.txt
@@ -12,6 +12,7 @@
AvoidNonConstGlobalVariablesCheck.cpp
AvoidReferenceCoroutineParametersCheck.cpp
CppCoreGuidelinesTidyModule.cpp
+ ForwardNonForwardingParameterCheck.cpp
InitVariablesCheck.cpp
InterfacesGlobalInitCheck.cpp
MacroUsageCheck.cpp
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
