[clang-tidy] modernize-avoid-bind only return for non-void function #69207

5chmidti · 2023-10-16T14:01:03Z

only return when the return type is non-void
fix missing return on member functions

- only return when the return type is non-void - fix missing return on member functions

llvmbot · 2023-10-16T14:02:43Z

@llvm/pr-subscribers-clang-tidy

Author: None (5chmidti)

Changes

only return when the return type is non-void
fix missing return on member functions

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

2 Files Affected:

(modified) clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp (+10-3)
(modified) clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp (+31-12)

diff --git a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
index 4628c09f196996b..bcdf6f98055486e 100644
--- a/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
+++ b/clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp
@@ -96,6 +96,7 @@ struct CallableInfo {
   std::string UsageIdentifier;
   StringRef CaptureInitializer;
   const FunctionDecl *Decl = nullptr;
+  bool DoesReturn = false;
 };
 
 struct LambdaProperties {
@@ -554,6 +555,8 @@ getLambdaProperties(const MatchFinder::MatchResult &Result) {
   LP.Callable.Materialization = getCallableMaterialization(Result);
   LP.Callable.Decl =
       getCallMethodDecl(Result, LP.Callable.Type, LP.Callable.Materialization);
+  if (LP.Callable.Decl)
+    LP.Callable.DoesReturn = !LP.Callable.Decl->getReturnType()->isVoidType();
   LP.Callable.SourceTokens = getSourceTextForExpr(Result, CalleeExpr);
   if (LP.Callable.Materialization == CMK_VariableRef) {
     LP.Callable.CE = CE_Var;
@@ -672,15 +675,20 @@ void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
 
   addPlaceholderArgs(LP, Stream, PermissiveParameterList);
 
+  Stream << " { ";
+
+  if (LP.Callable.DoesReturn) {
+    Stream << "return ";
+  }
+
   if (LP.Callable.Type == CT_Function) {
     StringRef SourceTokens = LP.Callable.SourceTokens;
     SourceTokens.consume_front("&");
-    Stream << " { return " << SourceTokens;
+    Stream << SourceTokens;
   } else if (LP.Callable.Type == CT_MemberFunction) {
     const auto *MethodDecl = dyn_cast<CXXMethodDecl>(LP.Callable.Decl);
     const BindArgument &ObjPtr = FunctionCallArgs.front();
 
-    Stream << " { ";
     if (!isa<CXXThisExpr>(ignoreTemporariesAndPointers(ObjPtr.E))) {
       Stream << ObjPtr.UsageIdentifier;
       Stream << "->";
@@ -688,7 +696,6 @@ void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
 
     Stream << MethodDecl->getName();
   } else {
-    Stream << " { return ";
     switch (LP.Callable.CE) {
     case CE_Var:
       if (LP.Callable.UsageIdentifier != LP.Callable.CaptureIdentifier) {
diff --git a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp
index 3334eab2d407e49..336b3cb4ee08f94 100644
--- a/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp
+++ b/clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp
@@ -45,6 +45,7 @@ struct D {
   void operator()(int x, int y) const {}
 
   void MemberFunction(int x) {}
+  int MemberFunctionWithReturn(int x) {}
 
   static D *create();
 };
@@ -162,11 +163,11 @@ struct TestCaptureByValueStruct {
     // those here.
     auto FFF = boost::bind(UseF, f);
     // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
-    // CHECK-FIXES: auto FFF = [f] { return UseF(f); };
+    // CHECK-FIXES: auto FFF = [f] { UseF(f); };
 
     auto GGG = boost::bind(UseF, MemberStruct);
     // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to boost::bind [modernize-avoid-bind]
-    // CHECK-FIXES: auto GGG = [this] { return UseF(MemberStruct); };
+    // CHECK-FIXES: auto GGG = [this] { UseF(MemberStruct); };
 
     auto HHH = std::bind(add, MemberStructWithData._member, 1);
     // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
@@ -227,34 +228,34 @@ void testFunctionObjects() {
   D *e = nullptr;
   auto AAA = std::bind(d, 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto AAA = [d] { return d(1, 2); }
+  // CHECK-FIXES: auto AAA = [d] { d(1, 2); }
 
   auto BBB = std::bind(*e, 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto BBB = [e] { return (*e)(1, 2); }
+  // CHECK-FIXES: auto BBB = [e] { (*e)(1, 2); }
 
   auto CCC = std::bind(D{}, 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto CCC = [] { return D{}(1, 2); }
+  // CHECK-FIXES: auto CCC = [] { D{}(1, 2); }
 
   auto DDD = std::bind(D(), 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto DDD = [] { return D()(1, 2); }
+  // CHECK-FIXES: auto DDD = [] { D()(1, 2); }
 
   auto EEE = std::bind(*D::create(), 1, 2);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: auto EEE = [Func = *D::create()] { return Func(1, 2); };
+  // CHECK-FIXES: auto EEE = [Func = *D::create()] { Func(1, 2); };
 
   auto FFF = std::bind(G(), 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // Templated function call operators may be used
-  // CHECK-FIXES: auto FFF = [] { return G()(1); };
+  // CHECK-FIXES: auto FFF = [] { G()(1); };
 
   int CTorArg = 42;
   auto GGG = std::bind(G(CTorArg), 1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // Function objects with constructor arguments should be captured
-  // CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { return Func(1); };
+  // CHECK-FIXES: auto GGG = [Func = G(CTorArg)] { Func(1); };
 }
 
 template <typename T>
@@ -262,7 +263,7 @@ void testMemberFnOfClassTemplate(T) {
   auto HHH = std::bind(H<T>(), 42);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // Ensure function class template arguments are preserved
-  // CHECK-FIXES: auto HHH = [] { return H<T>()(42); };
+  // CHECK-FIXES: auto HHH = [] { H<T>()(42); };
 }
 
 template void testMemberFnOfClassTemplate(int);
@@ -335,11 +336,12 @@ void testCapturedSubexpressions() {
   auto CCC = std::bind(sub, std::ref(*p), _1);
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
   // Expressions returning references are captured
-  // CHECK-FIXES: auto CCC = [&capture0 = *p](auto && PH1) { return sub(capture0, std::forward<decltype(PH1)>(PH1)); };
+  // CHECK-FIXES: auto CCC = [&capture0 = *p](auto && PH1) { sub(capture0, std::forward<decltype(PH1)>(PH1)); };
 }
 
 struct E {
   void MemberFunction(int x) {}
+  int MemberFunctionWithReturn(int x) {}
 
   void testMemberFunctions() {
     D *d;
@@ -360,6 +362,23 @@ struct E {
     auto DDD = std::bind(&D::MemberFunction, _1, 1);
     // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
     // CHECK-FIXES: auto DDD = [](auto && PH1) { PH1->MemberFunction(1); };
+
+    auto EEE = std::bind(&D::MemberFunctionWithReturn, d, 1);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto EEE = [d] { return d->MemberFunctionWithReturn(1); };
+
+    auto FFF = std::bind(&D::MemberFunctionWithReturn, &dd, 1);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto FFF = [ObjectPtr = &dd] { return ObjectPtr->MemberFunctionWithReturn(1); };
+
+    auto GGG = std::bind(&E::MemberFunctionWithReturn, this, 1);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto GGG = [this] { return MemberFunctionWithReturn(1); };
+
+    // Test what happens when the object pointer is itself a placeholder.
+    auto HHH = std::bind(&D::MemberFunctionWithReturn, _1, 1);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto HHH = [](auto && PH1) { return PH1->MemberFunctionWithReturn(1); };
   }
 };
 
@@ -368,5 +387,5 @@ void testStdFunction(Thing *t) {
   if (t)
     cb.Reset(std::bind(UseThing, t));
   // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind
-  // CHECK-FIXES: cb.Reset([t] { return UseThing(t); });
+  // CHECK-FIXES: cb.Reset([t] { UseThing(t); });
 }

clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp

PiotrZSL

Release notes.... Except that looks +- fine for me.

5chmidti · 2023-10-16T15:19:18Z

Any particular reason for the +-? Maybe it can be resolved?

clang-tools-extra/docs/ReleaseNotes.rst

PiotrZSL

Release notes still need some work, except that looks fine.

PiotrZSL · 2023-10-20T07:27:38Z

Any particular reason for the +-? Maybe it can be resolved?

I'm just not too familiar with this check.
But current change looks fine, and could be merged.

5chmidti · 2023-10-20T19:10:30Z

Well, I have two more PRs planned for this check :). If you feel like it, you can merge this.
I've checked to see if anyone from the main file's git blame (of functional changes) are active (involves:abc search for issues and prs), but they aren't really. The most recent involvement from one person is two years ago, and their contribution to AvoidBind.cpp was three years ago. I'm not sure if we should ping that one person.

[clang-tidy] modernize-avoid-bind only return for non-void function

7771acd

- only return when the return type is non-void - fix missing return on member functions

llvmbot added the clang-tidy label Oct 16, 2023

PiotrZSL reviewed Oct 16, 2023

View reviewed changes

clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp Outdated Show resolved Hide resolved

PiotrZSL reviewed Oct 16, 2023

View reviewed changes

address comments

8225a74

PiotrZSL reviewed Oct 16, 2023

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

5chmidti added 2 commits October 16, 2023 20:42

fix release note

3b770dd

fix missing getCanonicalType

14972fc

PiotrZSL reviewed Oct 16, 2023

View reviewed changes

clang-tools-extra/docs/ReleaseNotes.rst Outdated Show resolved Hide resolved

PiotrZSL approved these changes Oct 16, 2023

View reviewed changes

address release notes comment

5f4951a

PiotrZSL merged commit 9399094 into llvm:main Oct 20, 2023

spupyrev mentioned this pull request Oct 23, 2023

[BOLT] Rename cds to cdsort #69966

Merged

banach-space mentioned this pull request Oct 24, 2023

[mlir][vector] Add scalable vectors to tests for vector.contract #70039

Merged

5chmidti deleted the clang_tidy_modernize_avoid_bind_member_address_fix_returns branch November 2, 2024 21:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[clang-tidy] modernize-avoid-bind only return for non-void function #69207

[clang-tidy] modernize-avoid-bind only return for non-void function #69207

Uh oh!

5chmidti commented Oct 16, 2023

Uh oh!

llvmbot commented Oct 16, 2023

Uh oh!

Uh oh!

PiotrZSL left a comment

Uh oh!

5chmidti commented Oct 16, 2023

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Uh oh!

PiotrZSL commented Oct 20, 2023

Uh oh!

5chmidti commented Oct 20, 2023

Uh oh!

Uh oh!

[clang-tidy] modernize-avoid-bind only return for non-void function #69207

[clang-tidy] modernize-avoid-bind only return for non-void function #69207

Uh oh!

Conversation

5chmidti commented Oct 16, 2023

Uh oh!

llvmbot commented Oct 16, 2023

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

5chmidti commented Oct 16, 2023

Uh oh!

Uh oh!

Uh oh!

PiotrZSL left a comment

Choose a reason for hiding this comment

Uh oh!

PiotrZSL commented Oct 20, 2023

Uh oh!

5chmidti commented Oct 20, 2023

Uh oh!

Uh oh!