llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) <details> <summary>Changes</summary> Fixes: #<!-- -->83845 Capturing variable in lambda by reference and doing forward in lambda body are like constructing a struct by reference and using std::forward in some member function. It is dangerous if the lifetime of lambda expression is longer then current function. And it goes against what this check is intended to check. This PR wants to revert this behavior introduced in #<!-- -->77056 partially. --- Full diff: https://github.com/llvm/llvm-project/pull/83930.diff 3 Files Affected: - (modified) clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp (+5-52) - (modified) clang-tools-extra/docs/ReleaseNotes.rst (+3-1) - (modified) clang-tools-extra/test/clang-tidy/checkers/cppcoreguidelines/missing-std-forward.cpp (+26-23) ``````````diff diff --git a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp index c633683570f748..ecfad9cd44bfb1 100644 --- a/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp +++ b/clang-tools-extra/clang-tidy/cppcoreguidelines/MissingStdForwardCheck.cpp @@ -9,7 +9,6 @@ #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; @@ -53,68 +52,22 @@ AST_MATCHER(ParmVarDecl, isTemplateTypeParameter) { FuncTemplate->getTemplateParameters()->getDepth(); } -AST_MATCHER_P(NamedDecl, hasSameNameAsBoundNode, std::string, BindingID) { - IdentifierInfo *II = Node.getIdentifier(); - if (nullptr == II) - return false; - StringRef Name = II->getName(); - - return Builder->removeBindings( - [this, Name](const ast_matchers::internal::BoundNodesMap &Nodes) { - const DynTypedNode &BN = Nodes.getNode(this->BindingID); - if (const auto *ND = BN.get<NamedDecl>()) { - if (!isa<FieldDecl, CXXMethodDecl, VarDecl>(ND)) - return true; - return ND->getName() != Name; - } - return true; - }); -} - -AST_MATCHER_P(LambdaCapture, hasCaptureKind, LambdaCaptureKind, Kind) { - return Node.getCaptureKind() == Kind; -} - -AST_MATCHER_P(LambdaExpr, hasCaptureDefaultKind, LambdaCaptureDefault, Kind) { - return Node.getCaptureDefault() == Kind; -} - } // namespace void MissingStdForwardCheck::registerMatchers(MatchFinder *Finder) { - auto RefToParmImplicit = allOf( - equalsBoundNode("var"), hasInitializer(ignoringParenImpCasts( - declRefExpr(to(equalsBoundNode("param")))))); - auto RefToParm = capturesVar( - varDecl(anyOf(hasSameNameAsBoundNode("param"), RefToParmImplicit))); - auto HasRefToParm = hasAnyCapture(RefToParm); - - auto CaptureInRef = - allOf(hasCaptureDefaultKind(LambdaCaptureDefault::LCD_ByRef), - unless(hasAnyCapture( - capturesVar(varDecl(hasSameNameAsBoundNode("param")))))); - auto CaptureInCopy = allOf( - hasCaptureDefaultKind(LambdaCaptureDefault::LCD_ByCopy), HasRefToParm); - auto CaptureByRefExplicit = hasAnyCapture( - allOf(hasCaptureKind(LambdaCaptureKind::LCK_ByRef), RefToParm)); - - auto CapturedInBody = - lambdaExpr(anyOf(CaptureInRef, CaptureInCopy, CaptureByRefExplicit)); - auto CapturedInCaptureList = hasAnyCapture(capturesVar( - varDecl(hasInitializer(ignoringParenImpCasts(equalsBoundNode("call")))))); - auto CapturedInLambda = hasDeclContext(cxxRecordDecl( isLambda(), - hasParent(lambdaExpr(forCallable(equalsBoundNode("func")), - anyOf(CapturedInCaptureList, CapturedInBody))))); + hasParent( + lambdaExpr(forCallable(equalsBoundNode("func")), + hasAnyCapture(capturesVar(varDecl(hasInitializer( + ignoringParenImpCasts(equalsBoundNode("call")))))))))); auto ToParam = hasAnyParameter(parmVarDecl(equalsBoundNode("param"))); auto ForwardCallMatcher = callExpr( callExpr().bind("call"), argumentCountIs(1), hasArgument( - 0, declRefExpr(to( - varDecl(optionally(equalsBoundNode("param"))).bind("var")))), + 0, declRefExpr(to(varDecl(equalsBoundNode("param")).bind("var")))), forCallable(anyOf(equalsBoundNode("func"), CapturedInLambda)), callee(unresolvedLookupExpr(hasAnyDeclaration( namedDecl(hasUnderlyingDecl(hasName("::std::forward")))))), diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst index 0d2467210fc664..feac60491dda8e 100644 --- a/clang-tools-extra/docs/ReleaseNotes.rst +++ b/clang-tools-extra/docs/ReleaseNotes.rst @@ -142,7 +142,9 @@ Changes in existing checks - Improved :doc:`cppcoreguidelines-missing-std-forward <clang-tidy/checks/cppcoreguidelines/missing-std-forward>` check by no longer - giving false positives for deleted functions. + giving false positives for deleted functions, avoiding false negative when + multiple arguments in a function, reporting diagnostics when using forward in + body of lambdas. - Cleaned up :doc:`cppcoreguidelines-prefer-member-initializer <clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>` 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 index 20e43f04180ff3..3bf16a401682d5 100644 --- 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 @@ -75,6 +75,11 @@ class AClass { T data; }; +template <typename X, typename Y> void foo(X &&x, Y &&y) { + // CHECK-MESSAGES: :[[@LINE-1]]:55: warning: forwarding reference parameter 'y' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + auto newx = std::forward<X>(x); +} + template <class T> 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] @@ -90,9 +95,27 @@ void lambda_value_capture(T&& t) { } template <class T> -void lambda_value_capture_copy(T&& t) { - // CHECK-MESSAGES: :[[@LINE-1]]:36: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] - [&,t]() { T other = std::forward<T>(t); }; +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>(t); }; +} + +template<typename T> +void lambda_value_reference_capture_list_ref_1(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:52: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [=, &t] { T other = std::forward<T>(t); }; +} + +template<typename T> +void lambda_value_reference_capture_list_ref_2(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:52: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&t] { T other = std::forward<T>(t); }; +} + +template <class T> +void lambda_value_reference_auxiliary_var(T&& t) { + // CHECK-MESSAGES: :[[@LINE-1]]:47: warning: forwarding reference parameter 't' is never forwarded inside the function body [cppcoreguidelines-missing-std-forward] + [&x = t]() { T other = std::forward<T>(x); }; } } // namespace positive_cases @@ -147,31 +170,11 @@ class AClass { T data; }; -template <class T> -void lambda_value_reference(T&& t) { - [&]() { T other = std::forward<T>(t); }; -} - -template<typename T> -void lambda_value_reference_capture_list_ref_1(T&& t) { - [=, &t] { T other = std::forward<T>(t); }; -} - -template<typename T> -void lambda_value_reference_capture_list_ref_2(T&& t) { - [&t] { T other = std::forward<T>(t); }; -} - template<typename T> void lambda_value_reference_capture_list(T&& t) { [t = std::forward<T>(t)] { t(); }; } -template <class T> -void lambda_value_reference_auxiliary_var(T&& t) { - [&x = t]() { T other = std::forward<T>(x); }; -} - } // namespace negative_cases namespace deleted_functions { `````````` </details> https://github.com/llvm/llvm-project/pull/83930 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
