Skip to content

[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

Merged
merged 5 commits into from
Jun 8, 2025

Conversation

flovent
Copy link
Contributor

@flovent flovent commented May 25, 2025

Fixes #141381.

Add check for DeclRefExpr which points to an explicit object parameter.

@flovent flovent changed the title [clang-tidy] fix false positives with deducing this in readability-convert-member-functions-to-static check [clang-tidy] Fix false positives with deducing this in readability-convert-member-functions-to-static check May 25, 2025
@llvmbot
Copy link
Member

llvmbot commented May 25, 2025

@llvm/pr-subscribers-clang-tools-extra

Author: None (flovent)

Changes

Fixes #141381.

Add check for DeclRefExpr which points to an explicit object parameter.


Full diff: https://github.com/llvm/llvm-project/pull/141391.diff

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp (+10)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (added) clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp (+14)
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_); }
+};
+}

@llvmbot
Copy link
Member

llvmbot commented May 25, 2025

@llvm/pr-subscribers-clang-tidy

Author: None (flovent)

Changes

Fixes #141381.

Add check for DeclRefExpr which points to an explicit object parameter.


Full diff: https://github.com/llvm/llvm-project/pull/141391.diff

3 Files Affected:

  • (modified) clang-tools-extra/clang-tidy/readability/ConvertMemberFunctionsToStatic.cpp (+10)
  • (modified) clang-tools-extra/docs/ReleaseNotes.rst (+4)
  • (added) clang-tools-extra/test/clang-tidy/checkers/readability/convert-member-functions-to-static-deducing-this.cpp (+14)
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_); }
+};
+}

Copy link
Contributor

@vbvictor vbvictor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@flovent flovent force-pushed the clang-tidy-issue-141381 branch from f86e0c0 to 4d7edc0 Compare May 25, 2025 10:00
@flovent flovent force-pushed the clang-tidy-issue-141381 branch from 4d7edc0 to c9f00ba Compare May 27, 2025 12:34
@vbvictor
Copy link
Contributor

vbvictor commented Jun 8, 2025

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?

@flovent flovent force-pushed the clang-tidy-issue-141381 branch from c9f00ba to 872e07c Compare June 8, 2025 06:18
@flovent
Copy link
Contributor Author

flovent commented Jun 8, 2025

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?

Copy link
Contributor

@carlosgalvezp carlosgalvezp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@vbvictor
Copy link
Contributor

vbvictor commented Jun 8, 2025

When merging, your new public email will be used, now it says
"This commit will be authored by flbven@protonmail.com."

So you don't have to do anything with your commits

@vbvictor vbvictor merged commit 239c8ac into llvm:main Jun 8, 2025
8 checks passed
@flovent flovent deleted the clang-tidy-issue-141381 branch June 8, 2025 07:30
rorth pushed a commit to rorth/llvm-project that referenced this pull request Jun 11, 2025
…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>
DhruvSrivastavaX pushed a commit to DhruvSrivastavaX/lldb-for-aix that referenced this pull request Jun 12, 2025
…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>
tomtor pushed a commit to tomtor/llvm-project that referenced this pull request Jun 14, 2025
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

False-positive readability-convert-member-functions-to-static when using explicit object parameters
4 participants