[clang-tidy] modernize-avoid-bind: Fix crash when method name is not a simple identifier

[jspam]

Summary

The following assertion fails when referencing an operator member:

include/clang/AST/Decl.h:275: llvm::StringRef clang::NamedDecl::getName() const: Assertion `Name.isIdentifier() && "Name is not a simple identifier"' failed.

Additionally, use the call syntax instead of "operator()" and fix
missing "return" keyword.

[njames93]

clang-tools-extra/test/clang-tidy/checkers/modernize-avoid-bind.cpp

367 ↗

(On Diff #430587)

Fixing a crash is good, but these fixes leave a lot to be desired. If you could handle the cases when the name is an operator call by actually using the operator it would be be a lot nicer.

auto EEE = [d] { d(1, 2); };

This definitely could be done in a follow up commit.

[jspam]

Improve fixes

As suggested by njames93, use call syntax instead of operator().

[PiotrZSL]

Crash no longer happens on main, but this change introduce nice fix for operators handling.

[PiotrZSL]

Rebase

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

void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
   if (LP.Callable.Type == CT_Function) {
     StringRef SourceTokens = LP.Callable.SourceTokens;
     SourceTokens.consume_front("&");
     Stream << SourceTokens;
   } else if (LP.Callable.Type == CT_MemberFunction) {
     const auto *MethodDecl = dyn_cast<CXXMethodDecl>(LP.Callable.Decl);
     const BindArgument &ObjPtr = FunctionCallArgs.front();
 
+    if (MethodDecl->getOverloadedOperator() == OO_Call) {
+      Stream << "(*" << ObjPtr.UsageIdentifier << ')';
+    } else {
       if (!isa<CXXThisExpr>(ignoreTemporariesAndPointers(ObjPtr.E))) {
         Stream << ObjPtr.UsageIdentifier;
         Stream << "->";
       }
-
-    Stream << MethodDecl->getName();
+      Stream << MethodDecl->getNameAsString();
+    }
   } else {
     switch (LP.Callable.CE) {
     case CE_Var:
       if (LP.Callable.UsageIdentifier != LP.Callable.CaptureIdentifier) {
         Stream << "(" << LP.Callable.UsageIdentifier << ")";
         break;
       }
       [[fallthrough]];

clang-tools-extra/test/clang-tidy/checkers/modernize/avoid-bind.cpp

 
 struct Foo {
   static int add(int x, int y) { return x + y; }
 };
 
 struct D {
   D() = default;
   void operator()(int x, int y) const {}
+  operator bool() const { return true; }
 
   void MemberFunction(int x) {}
   int MemberFunctionWithReturn(int x) {}
 
   static D *create();
 };
 
 struct F {
void testCapturedSubexpressions() {
   // 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) { sub(capture0, std::forward<decltype(PH1)>(PH1)); };
 }
 
 struct E {
   void MemberFunction(int x) {}
   int MemberFunctionWithReturn(int x) {}
+  int operator()(int x, int y) const { return x + y; }
 
   void testMemberFunctions() {
     D *d;
     D dd;
     auto AAA = std::bind(&D::MemberFunction, d, 1);
     // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
     // CHECK-FIXES: auto AAA = [d] { d->MemberFunction(1); };
 
void testMemberFunctions() {
     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); };
+
+    auto III = std::bind(&D::operator(), d, 1, 2);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto III = [d] { (*d)(1, 2); }
+
+    auto JJJ = std::bind(&D::operator(), &dd, 1, 2);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto JJJ = [ObjectPtr = &dd] { (*ObjectPtr)(1, 2); }
+
+    auto KKK = std::bind(&D::operator(), _1, 1, 2);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto KKK = [](auto && PH1) { (*PH1)(1, 2); };
+
+    auto LLL = std::bind(&D::operator bool, d);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto LLL = [d] { return d->operator bool(); }
+
+    auto MMM = std::bind(&E::operator(), this, 1, 2);
+    // CHECK-MESSAGES: :[[@LINE-1]]:16: warning: prefer a lambda to std::bind
+    // CHECK-FIXES: auto MMM = [this] { return (*this)(1, 2); }
   }
 };
 
 void testStdFunction(Thing *t) {
   Callback cb;
   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] { UseThing(t); });
 }