[clang-tidy][missing-std-forward]report diagnotics for using forward in lambda body #83930
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…in lambda body Fixes: llvm#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 llvm#77056 partially.
|
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: Congcong Cai (HerrCai0907) ChangesFixes: #83845 Full diff: https://github.com/llvm/llvm-project/pull/83930.diff 3 Files Affected:
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 {
|
HerrCai0907
deleted the
83845-clang-tidy-doesnt-report-missing-forward-when-one-parameter-is-forwarded-but-some-other-parameter-isnt
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.