-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[clang-tidy] Fix false positives with deducing this in readability-convert-member-functions-to-static
check
#141391
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
readability-convert-member-functions-to-static
checkreadability-convert-member-functions-to-static
check
@llvm/pr-subscribers-clang-tools-extra Author: None (flovent) ChangesFixes #141381. Add check for Full diff: https://github.com/llvm/llvm-project/pull/141391.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
index 1284df6bd99cf..5cdd00414e01e 100644
--- a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
@@ -62,6 +62,16 @@ AST_MATCHER(CXXMethodDecl, usesThis) {
return false; // Stop traversal.
}
+ bool VisitDeclRefExpr(const DeclRefExpr *E) {
+ if (const auto *PVD = dyn_cast_if_present<ParmVarDecl>(E->getDecl());
+ PVD && PVD->isExplicitObjectParameter()) {
+ Used = true;
+ return false; // Stop traversal.
+ }
+
+ return true;
+ }
+
// If we enter a class declaration, don't traverse into it as any usages of
// `this` will correspond to the nested class.
bool TraverseCXXRecordDecl(CXXRecordDecl *RD) { return true; }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8611b5e0af272..a138272cc8e39 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -234,6 +234,10 @@ Changes in existing checks
tolerating fix-it breaking compilation when functions is used as pointers
to avoid matching usage of functions within the current compilation unit.
+- Improved :doc:`readability-convert-member-functions-to-static
+ <clang-tidy/checks/readability/convert-member-functions-to-static>` check by
+ fixing false positives on member functions with an explicit object parameter.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
new file mode 100644
index 0000000000000..bb9755abc9cfa
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
@@ -0,0 +1,14 @@
+// RUN: %check_clang_tidy -std=c++23 %s readability-convert-member-functions-to-static %t
+
+namespace std{
+ class string {};
+ void println(const char *format, const std::string &str) {}
+}
+
+namespace PR141381 {
+struct Hello {
+ std::string str_;
+
+ void hello(this Hello &self) { std::println("Hello, {0}!", self.str_); }
+};
+}
|
@llvm/pr-subscribers-clang-tidy Author: None (flovent) ChangesFixes #141381. Add check for Full diff: https://github.com/llvm/llvm-project/pull/141391.diff 3 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
index 1284df6bd99cf..5cdd00414e01e 100644
--- a/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
+++ b/clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
@@ -62,6 +62,16 @@ AST_MATCHER(CXXMethodDecl, usesThis) {
return false; // Stop traversal.
}
+ bool VisitDeclRefExpr(const DeclRefExpr *E) {
+ if (const auto *PVD = dyn_cast_if_present<ParmVarDecl>(E->getDecl());
+ PVD && PVD->isExplicitObjectParameter()) {
+ Used = true;
+ return false; // Stop traversal.
+ }
+
+ return true;
+ }
+
// If we enter a class declaration, don't traverse into it as any usages of
// `this` will correspond to the nested class.
bool TraverseCXXRecordDecl(CXXRecordDecl *RD) { return true; }
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 8611b5e0af272..a138272cc8e39 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -234,6 +234,10 @@ Changes in existing checks
tolerating fix-it breaking compilation when functions is used as pointers
to avoid matching usage of functions within the current compilation unit.
+- Improved :doc:`readability-convert-member-functions-to-static
+ <clang-tidy/checks/readability/convert-member-functions-to-static>` check by
+ fixing false positives on member functions with an explicit object parameter.
+
Removed checks
^^^^^^^^^^^^^^
diff --git a/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp b/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
new file mode 100644
index 0000000000000..bb9755abc9cfa
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
@@ -0,0 +1,14 @@
+// RUN: %check_clang_tidy -std=c++23 %s readability-convert-member-functions-to-static %t
+
+namespace std{
+ class string {};
+ void println(const char *format, const std::string &str) {}
+}
+
+namespace PR141381 {
+struct Hello {
+ std::string str_;
+
+ void hello(this Hello &self) { std::println("Hello, {0}!", self.str_); }
+};
+}
|
clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
Outdated
Show resolved
Hide resolved
...ra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
Outdated
Show resolved
Hide resolved
...ra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
Outdated
Show resolved
Hide resolved
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
clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp
Outdated
Show resolved
Hide resolved
...ra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp
Outdated
Show resolved
Hide resolved
f86e0c0
to
4d7edc0
Compare
4d7edc0
to
c9f00ba
Compare
Could you please rebase on fresh main, and I will land the PR. PS. Sorry for mentioning it again, but I've seen a github runner about private email on you other PR and It's a developer policy to have it public (https://llvm.org/docs/DeveloperPolicy.html#email-addresses). If you don't have strong privacy concerns of revealing your email, could you please do it? |
c9f00ba
to
872e07c
Compare
Rebased and changed my email to public. Different with this PR, I use github noreply email in my latest PR to commit, do I need to rebase to change it or it will automaticly update to my public email when merging like it will transfter to noreply email when i keep it private like LLVM Discourse says? |
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!
When merging, your new public email will be used, now it says So you don't have to do anything with your commits |
…onvert-member-functions-to-static` check (llvm#141391) Add check for `DeclRefExpr` which points to an explicit object parameter. Fixes llvm#141381. --------- Co-authored-by: fubowen <fubowen@protomail.com> Co-authored-by: flovent <flbven@protomail.com>
…onvert-member-functions-to-static` check (llvm#141391) Add check for `DeclRefExpr` which points to an explicit object parameter. Fixes llvm#141381. --------- Co-authored-by: fubowen <fubowen@protomail.com> Co-authored-by: flovent <flbven@protomail.com>
…onvert-member-functions-to-static` check (llvm#141391) Add check for `DeclRefExpr` which points to an explicit object parameter. Fixes llvm#141381. --------- Co-authored-by: fubowen <fubowen@protomail.com> Co-authored-by: flovent <flbven@protomail.com>
Fixes #141381.
Add check for
DeclRefExpr
which points to an explicit object parameter.