-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
[clang-tidy] Let bugprone-use-after-move
also handle calls to std::forward
#82673
Conversation
@llvm/pr-subscribers-clang-tidy @llvm/pr-subscribers-clang-tools-extra Author: None (AMS21) ChangesFixes #82023 Full diff: https://github.com/llvm/llvm-project/pull/82673.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
index c5b6b541096ca9..f2eaebb7e57833 100644
--- a/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
+++ b/clang-tools-extra/clang-tidy/bugprone/UseAfterMoveCheck.cpp
@@ -330,7 +330,7 @@ void UseAfterMoveFinder::getReinits(
traverse(TK_AsIs, DeclRefMatcher),
unless(parmVarDecl(hasType(
references(qualType(isConstQualified())))))),
- unless(callee(functionDecl(hasName("::std::move")))))))
+ unless(callee(functionDecl(hasAnyName("::std::move", "::std::forward")))))))
.bind("reinit");
Stmts->clear();
@@ -388,7 +388,7 @@ void UseAfterMoveCheck::registerMatchers(MatchFinder *Finder) {
auto TryEmplaceMatcher =
cxxMemberCallExpr(callee(cxxMethodDecl(hasName("try_emplace"))));
auto CallMoveMatcher =
- callExpr(argumentCountIs(1), callee(functionDecl(hasName("::std::move"))),
+ callExpr(argumentCountIs(1), callee(functionDecl(hasAnyName("::std::move", "::std::forward"))),
hasArgument(0, declRefExpr().bind("arg")),
unless(inDecltypeOrTemplateArg()),
unless(hasParent(TryEmplaceMatcher)), expr().bind("call-move"),
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index a0b9fcfe0d7774..55c47c7617ce2a 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -130,6 +130,10 @@ Changes in existing checks
<clang-tidy/checks/bugprone/unused-local-non-trivial-variable>` check by
ignoring local variable with ``[maybe_unused]`` attribute.
+- Improved :doc:`bugprone-use-after-move
+ <clang-tidy/checks/bugprone/use-after-move>` check to also handle
+ calls to ``std::forward``.
+
- Cleaned up :doc:`cppcoreguidelines-prefer-member-initializer
<clang-tidy/checks/cppcoreguidelines/prefer-member-initializer>`
by removing enforcement of rule `C.48
diff --git a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
index 00b1da1e727e4f..0138064ed811b5 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/bugprone/use-after-move.cpp
@@ -111,6 +111,18 @@ constexpr typename std::remove_reference<_Tp>::type &&move(_Tp &&__t) noexcept {
return static_cast<typename remove_reference<_Tp>::type &&>(__t);
}
+template <class _Tp>
+constexpr _Tp&&
+forward(typename std::remove_reference<_Tp>::type& __t) noexcept {
+ return static_cast<_Tp&&>(__t);
+}
+
+template <class _Tp>
+constexpr _Tp&&
+forward(typename std::remove_reference<_Tp>::type&& __t) noexcept {
+ return static_cast<_Tp&&>(__t);
+}
+
} // namespace std
class A {
@@ -1525,3 +1537,28 @@ class PR38187 {
private:
std::string val_;
};
+
+namespace issue82023
+{
+
+struct S {
+ S();
+ S(S&&);
+};
+
+void consume(S s);
+
+template <typename T>
+void forward(T&& t) {
+ consume(std::forward<T>(t));
+ consume(std::forward<T>(t));
+ // CHECK-NOTES: [[@LINE-1]]:27: warning: 't' used after it was moved
+ // CHECK-NOTES: [[@LINE-3]]:11: note: move occurred here
+}
+
+void create() {
+ S s;
+ forward(std::move(s));
+}
+
+} // namespace issue82023
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
I'd like to see the diagnostics say |
Rebased and address review comment |
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 a short explanation that forward
is also checked in the check's docs here https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/docs/clang-tidy/checks/bugprone/use-after-move.rst.
Rebased and implemented review suggestions |
Rebased to fix merge conflict |
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.
Overall, LGTM
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.
LGTM
Fixes #82023