-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[clang-tidy] fix false positive in bugprone-return-const-ref-from-parameter #117734
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-tidy Author: Qizhi Hu (jcsxky) ChangesFix #115743 Full diff: https://github.com/llvm/llvm-project/pull/117734.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
index adb26ade955c5e..874f35f055094e 100644
--- a/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/ReturnConstRefFromParameterCheck.cpp
@@ -21,10 +21,10 @@ void ReturnConstRefFromParameterCheck::registerMatchers(MatchFinder *Finder) {
to(parmVarDecl(hasType(hasCanonicalType(
qualType(lValueReferenceType(pointee(
qualType(isConstQualified()))))
- .bind("type"))))
+ .bind("type"))), parmVarDecl(hasDeclContext(functionDecl().bind("owner"))))
.bind("param")))),
hasAncestor(
- functionDecl(hasReturnTypeLoc(loc(qualType(
+ functionDecl(equalsBoundNode("owner"), hasReturnTypeLoc(loc(qualType(
hasCanonicalType(equalsBoundNode("type"))))))
.bind("func")))
.bind("ret"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index b72d109b3d3938..9c3a5e4f40f930 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -104,6 +104,10 @@ New check aliases
Changes in existing checks
^^^^^^^^^^^^^^^^^^^^^^^^^^
+- Improved :doc:`bugprone-return-const-ref-from-parameter
+ <clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check by no longer
+ giving false positives for lambda.
+
- Improved :doc:`readability-redundant-smartptr-get
<clang-tidy/checks/readability/redundant-smartptr-get>` check to
remove `->`, when reduntant `get()` is removed.
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
index d13c127da7c2a1..391e8f96401ab4 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/return-const-ref-from-parameter.cpp
@@ -142,6 +142,12 @@ void instantiate(const int ¶m, const float ¶mf, int &mut_param, float &m
itf6(mut_paramf);
}
+template<class T>
+void f(const T& t) {
+ const auto get = [&t] -> const T& { return t; };
+ return T{};
+}
+
} // namespace valid
namespace overload {
|
Please rebase from current |
37a518b
to
80e12b2
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
7dd899e
to
5d35398
Compare
void f(const T& t) { | ||
const auto get = [&t] -> const T& { return t; }; | ||
return T{}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add test:
const auto get = [](const T& t2) -> const T& { return t2; };
and verify that it's being still detected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
qualType(lValueReferenceType( | ||
pointee(qualType(isConstQualified())))) | ||
.bind("type"))), | ||
parmVarDecl(hasDeclContext(functionDecl().bind("owner")))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for duplicating parmVarDecl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add positive test for lambdas.
5d35398
to
7100846
Compare
Positive test cases have been added. |
7100846
to
0a7192d
Compare
@@ -179,7 +179,8 @@ Changes in existing checks | |||
- Improved :doc:`bugprone-return-const-ref-from-parameter | |||
<clang-tidy/checks/bugprone/return-const-ref-from-parameter>` check to | |||
diagnose potential dangling references when returning a ``const &`` parameter | |||
by using the conditional operator ``cond ? var1 : var2``. | |||
by using the conditional operator ``cond ? var1 : var2`` and no longer giving |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it is a fix of false positives. Maybe in generic, it more looks like add support of lambda function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function in this issue contains lambda and thus gives a false positive(because it should not report diagnose since the return statement is not belongs to current function). I have modified the release note and make it easier to be understood.
0a7192d
to
528bc58
Compare
Fix #115743