clang-tidy: avoid std::bind

[jbcoe]

Summary

Replace std::bind with a lambda.

Not yet working for member functions.

[jbcoe]

Placeholder handling needs correcting.

[jbcoe]

Require C++14 for improved fixits.

Do not attempt to generate fixits for more complicated uses of bind.

[jbcoe]

Moved check to readability module.

Aside: would it be worthwhile creating a python script to move checks from one module to another?

[alexfh]

In D16962#346822, @jbcoe wrote:

Moved check to readability module.

Aside: would it be worthwhile creating a python script to move checks from one module to another?

It's reasonable to teach the rename_check.py script move across modules.

(Substantial comments later)

[alexfh]

clang-tidy/readability/AvoidStdBindCheck.cpp

37 ↗

(On Diff #47259)

SmallVector<> would be better here, since the number of arguments is usually rather low.

38 ↗

(On Diff #47259)

Please make the functions static.

42 ↗

(On Diff #47259)

Please use a range-based for loop over C->arguments().

45 ↗

(On Diff #47259)

Should be const auto *M to make it clear M is a pointer. Same for other code like this.

67 ↗

(On Diff #47259)

Please use ArrayRef<> to pass a constant reference to a sequence of elements.

100 ↗

(On Diff #47259)

I don't like the asymmetry here: the opening parenthesis is added in the caller and the closing parenthesis is added here.

104 ↗

(On Diff #47259)

llvm::SmallPtrSet<size_t> PlaceHolderIndices;
for (const BindArgument &B : Args) {
  if (B.PlaceHolderIndex) {
    if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second)
      return true;
  }
}
return false;

126 ↗

(On Diff #47259)

Many other checks use just Diag.

127 ↗

(On Diff #47259)

Should the message recommend something instead?

127 ↗

(On Diff #47259)

In pre-C++11 code the check will just warn without suggesting any alternative. That will lead to a lot of user confusion. We either need to restrict the warning to C++14 code or suggest a better alternative even in pre-C++14 code.

129 ↗

(On Diff #47259)

nit: Trailing period.

139 ↗

(On Diff #47259)

This would be shorter with std::any_of or even better with llvm::any_of that accepts a range instead of begin/end.

163 ↗

(On Diff #47259)

Use llvm::any_of.

clang-tidy/readability/AvoidStdBindCheck.h

1 ↗

(On Diff #47259)

Join with the next line and remove extra dashes, please.

docs/clang-tidy/checks/readability-avoid-std-bind.rst

4 ↗

(On Diff #47259)

nit: Add moar ekwality.

6 ↗

(On Diff #47259)

Use double backquotes to highlight inline code snippets like std::bind.

[jbcoe]

Apply requested fixes from review.

Thanks for taking time to review this, the patch is much improved for the attention.

[jbcoe]

clang-tidy/readability/AvoidStdBindCheck.cpp

42 ↗

(On Diff #47259)

I'm starting at 1 not zero, hence the explicit loop.

Elements are non-contiguous so I can't use a restricted ArrayView.

I've added a comment saying why it starts at 1, not 0.

127 ↗

(On Diff #47259)

The message now recommends using a lambda so I think this is addressed.

[alexfh]

clang-tidy/readability/AvoidStdBindCheck.cpp

46 ↗

(On Diff #50623)

In LLVM style this should be const auto *M.
Please clang-format the file.

48 ↗

(On Diff #50623)

Please use isa instead of dyn_cast, when you don't need the pointer returned by dyn_cast.

49 ↗

(On Diff #50623)

I'd prefer to use the conditional operator here: B.Kind = isa<CallExpr>(TE) ? BK_CallExpr : BK_Temporary;.

116 ↗

(On Diff #50623)

You should be able to use hasName("std::bind") instead.

docs/clang-tidy/checks/readability-avoid-std-bind.rst

6 ↗

(On Diff #50623)

"The check finds uses of ..."

9 ↗

(On Diff #50623)

Add a comma before "not"?

11 ↗

(On Diff #50623)

Please add an example or two of what code is flagged and what suggested fixes are.

[aaron.ballman]

clang-tidy/readability/AvoidStdBindCheck.cpp

116 ↗

(On Diff #50623)

I think that has to be hasName("::std::bind") to support namespace mine { namespace std { void bind(); } }

126 ↗

(On Diff #50623)

Might as well move this to the top of check() since there's no need to create the diagnostic or find the matched decl unless this check passes.

clang-tidy/readability/ReadabilityTidyModule.cpp

37 ↗

(On Diff #50623)

I kind of wonder if this should be in modernize instead of readability? Tough call, and I'm fine with either place, just wanted to pose the question.

docs/clang-tidy/checks/readability-avoid-std-bind.rst

9 ↗

(On Diff #50623)

or "and not" instead of a comma, if verbosity is your thing.

test/clang-tidy/readability-avoid-std-bind.cpp

1 ↗

(On Diff #50623)

Can you clang-format this file as well?

21 ↗

(On Diff #50623)

This should immediately follow the CHECK-MESSAGES in the same compound statement (here and elsewhere).

[jbcoe]

Minor fixes from review.

clang-tidy/readability/ReadabilityTidyModule.cpp

37 ↗

(On Diff #50623)

Given that lambdas and std::bind both arrived in C++11, modernize seems a misnomer.
I think readability is better fit.

[jbcoe]

Remove unused function isStdBind.

[alexfh]

Please regenerate the patch with the full context (see http://llvm.org/docs/Phabricator.html).

clang-tidy/readability/AvoidStdBindCheck.cpp

77 ↗

(On Diff #55974)

clang-format, please.

116 ↗

(On Diff #55974)

This should be done in registerMatchers instead.

docs/clang-tidy/checks/readability-avoid-std-bind.rst

13 ↗

(On Diff #55974)

.. code:: should be followed by an empty line.

Please also verify the documentation can be built without errors. On Ubuntu this boils down to:

1. Install sphinx:
   
   $ sudo apt-get install sphinx-common

2. Enable LLVM_BUILD_DOCS and maybe some other options in cmake.

3. ninja docs-clang-tools-html (or something similar, if you use make).

[aaron.ballman]

clang-tidy/readability/ReadabilityTidyModule.cpp

17 ↗

(On Diff #55974)

I believe we use "modernize" to really mean "migrate from the old way to the new way", which this definitely fits into since I think the point to this check is to replace bind with better alternatives.

[jbcoe]

clang-tidy/readability/ReadabilityTidyModule.cpp

17 ↗

(On Diff #55974)

Would you prefer it to be in modernize? I can be easily convinced either way and am happy to move it. If I do move it I might add a script to facilitate doing so.

[aaron.ballman]

clang-tidy/readability/ReadabilityTidyModule.cpp

17 ↗

(On Diff #55974)

My preference is for modernize, your preference is for readability, so I say: make @alexfh the tie-breaker! ;-) Alex, what are your thoughts? This seems like a heuristic we may want to state in our documentation to help others decide where to put new checks in the future as well.

[jbcoe]

Move to modernize. Rename to avoid-bind as a later patch will add support for boost::bind, std::bind1st etc.

[alexfh]

clang-tidy/modernize/AvoidBindCheck.cpp

28

I wonder whether VS2013 supports inline member initializers?

78

clang-format, please. Also, please join the two string literals here.

117

This check should be done in registerMatchers to avoid unnecessary work.

162

I don't think you need getLocForEndOfToken, since FixItHint::CreateReplacement(SourceRange) treats the range as a token range.

docs/clang-tidy/checks/modernize-avoid-bind.rst

27

..code:: C++ directive should be followed by an empty line. Please check the documentation builds without warnings.

33

s/We created this check because //

docs/clang-tidy/checks/readability-avoid-std-bind.rst

13 ↗

(On Diff #55974)

This comment is not addressed.

test/clang-tidy/modernize-avoid-bind.cpp

26

Remove [modernize-avoid-bind] from all CHECK-MESSAGES lines except for the first one.

[jbcoe]

Apply fixes from review.

[alexfh]

Looks good. Thank you!

Do you need someone to submit the patch for you?

clang-tidy/modernize/AvoidBindCheck.cpp

29

Answering my own question: according to https://msdn.microsoft.com/en-us/library/hh567368.aspx non-static member initializers are supported by VS2013. This should be fine.

[jbcoe]

I can submit the patch myself, thanks.

Thanks again for taking the time to give such a thorough review.

[jbcoe]

update diff to allow arc to apply patch.

clang-tidy/modernize/AvoidBindCheck.h

+//===--- AvoidBindCheck.h - clang-tidy---------------------------*- C++ -*-===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H
+
+#include "../ClangTidy.h"
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+/// Replace simple uses of std::bind with a lambda.
+///
+/// FIXME: Add support for function references and member function references.
+///
+/// For the user-facing documentation see:
+/// http://clang.llvm.org/extra/clang-tidy/checks/modernize-avoid-std-bind.html
+class AvoidBindCheck : public ClangTidyCheck {
+public:
+  AvoidBindCheck(StringRef Name, ClangTidyContext *Context)
+      : ClangTidyCheck(Name, Context) {}
+  void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+  void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+};
+} // namespace modernize
+} // namespace tidy
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_MODERNIZE_AVOID_BIND_H

clang-tidy/modernize/AvoidBindCheck.cpp

+//===--- AvoidBindCheck.cpp - clang-tidy--------------------------------===//
+//
+//                     The LLVM Compiler Infrastructure
+//
+// This file is distributed under the University of Illinois Open Source
+// License. See LICENSE.TXT for details.
+//
+//===----------------------------------------------------------------------===//
+#include "AvoidBindCheck.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/Lex/Lexer.h"
+#include <cassert>
+#include <unordered_map>
+
+using namespace clang::ast_matchers;
+
+namespace clang {
+namespace tidy {
+namespace modernize {
+
+namespace {
+enum BindArgumentKind { BK_Temporary, BK_Placeholder, BK_CallExpr, BK_Other };
+
+struct BindArgument {
+  StringRef Tokens;
+  BindArgumentKind Kind = BK_Other;
+  size_t PlaceHolderIndex = 0;
+};
+
+} // end namespace
+
+static SmallVector<BindArgument, 4>
+buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) {
+  SmallVector<BindArgument, 4> BindArguments;
+  llvm::Regex MatchPlaceholder("^_([0-9]+)$");
+
+  // Start at index 1 as first argument to bind is the function name.
+  for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) {
+    const Expr *E = C->getArg(I);
+    BindArgument B;
+    if (const auto *M = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      const auto *TE = M->GetTemporaryExpr();
+      B.Kind = isa<CallExpr>(TE) ? BK_CallExpr : BK_Temporary;
+    }
+
+    B.Tokens = Lexer::getSourceText(
+        CharSourceRange::getTokenRange(E->getLocStart(), E->getLocEnd()),
+        *Result.SourceManager, Result.Context->getLangOpts());
+
+    SmallVector<StringRef, 2> Matches;
+    if (B.Kind == BK_Other && MatchPlaceholder.match(B.Tokens, &Matches)) {
+      B.Kind = BK_Placeholder;
+      B.PlaceHolderIndex = std::stoi(Matches[1]);
+    }
+    BindArguments.push_back(B);
+  }
+  return BindArguments;
+}
+
+static void addPlaceholderArgs(const ArrayRef<BindArgument> Args,
+                               llvm::raw_ostream &Stream) {
+  auto MaxPlaceholderIt =
+      std::max_element(Args.begin(), Args.end(),
+                       [](const BindArgument &B1, const BindArgument &B2) {
+                         return B1.PlaceHolderIndex < B2.PlaceHolderIndex;
+                       });
+
+  // Placeholders (if present) have index 1 or greater.
+  if (MaxPlaceholderIt == Args.end() || MaxPlaceholderIt->PlaceHolderIndex == 0)
+    return;
+
+  size_t PlaceholderCount = MaxPlaceholderIt->PlaceHolderIndex;
+  Stream << "(";
+  StringRef Delimiter = "";
+  for (size_t I = 1; I <= PlaceholderCount; ++I) {
+    Stream << Delimiter << "auto && arg" << I;
+    Delimiter = ", ";
+  }
+  Stream << ")";
+}
+
+static void addFunctionCallArgs(const ArrayRef<BindArgument> Args,
+                                llvm::raw_ostream &Stream) {
+  StringRef Delimiter = "";
+  for (const auto &B : Args) {
+    if (B.PlaceHolderIndex)
+      Stream << Delimiter << "arg" << B.PlaceHolderIndex;
+    else
+      Stream << Delimiter << B.Tokens;
+    Delimiter = ", ";
+  }
+}
+
+static bool isPlaceHolderIndexRepeated(const ArrayRef<BindArgument> Args) {
+  llvm::SmallSet<size_t, 4> PlaceHolderIndices;
+  for (const BindArgument &B : Args) {
+    if (B.PlaceHolderIndex) {
+      if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second)
+        return true;
+    }
+  }
+  return false;
+}
+
+void AvoidBindCheck::registerMatchers(MatchFinder *Finder) {
+  if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas.
+    return;
+  
+  Finder->addMatcher(
+      callExpr(callee(namedDecl(hasName("::std::bind"))),
+               hasArgument(0, declRefExpr(to(functionDecl().bind("f")))))
+          .bind("bind"),
+      this);
+}
+
+void AvoidBindCheck::check(const MatchFinder::MatchResult &Result) {
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("bind");
+  auto Diag = diag(MatchedDecl->getLocStart(), "prefer a lambda to std::bind");
+
+  const auto Args = buildBindArguments(Result, MatchedDecl);
+
+  // Do not attempt to create fixits for nested call expressions.
+  // FIXME: Create lambda capture variables to capture output of calls.
+  // NOTE: Supporting nested std::bind will be more difficult due to placeholder
+  // sharing between outer and inner std:bind invocations.
+  if (llvm::any_of(Args,
+                   [](const BindArgument &B) { return B.Kind == BK_CallExpr; }))
+    return;
+
+  // Do not attempt to create fixits when placeholders are reused.
+  // Unused placeholders are supported by requiring C++14 generic lambdas.
+  // FIXME: Support this case by deducing the common type.
+  if (isPlaceHolderIndexRepeated(Args))
+    return;
+
+  const auto *F = Result.Nodes.getNodeAs<FunctionDecl>("f");
+
+  // std::bind can support argument count mismatch between its arguments and the
+  // bound function's arguments. Do not attempt to generate a fixit for such
+  // cases.
+  // FIXME: Support this case by creating unused lambda capture variables.
+  if (F->getNumParams() != Args.size())
+    return;
+
+  std::string Buffer;
+  llvm::raw_string_ostream Stream(Buffer);
+
+  bool HasCapturedArgument = llvm::any_of(
+      Args, [](const BindArgument &B) { return B.Kind == BK_Other; });
+
+  Stream << "[" << (HasCapturedArgument ? "=" : "") << "]";
+  addPlaceholderArgs(Args, Stream);
+  Stream << " { return " << F->getName() << "(";
+  addFunctionCallArgs(Args, Stream);
+  Stream << "); };";
+
+  Diag << FixItHint::CreateReplacement(MatchedDecl->getSourceRange(), Stream.str());
+}
+
+} // namespace modernize
+} // namespace tidy
+} // namespace clang

clang-tidy/modernize/CMakeLists.txt

 set(LLVM_LINK_COMPONENTS support)
 
 add_clang_library(clangTidyModernizeModule
+  AvoidBindCheck.cpp
   DeprecatedHeadersCheck.cpp
   LoopConvertCheck.cpp
   LoopConvertUtils.cpp

clang-tidy/modernize/ModernizeTidyModule.cpp

 #include "../ClangTidy.h"
 #include "../ClangTidyModule.h"
 #include "../ClangTidyModuleRegistry.h"
+#include "AvoidBindCheck.h"
 #include "DeprecatedHeadersCheck.h"
 #include "LoopConvertCheck.h"
 #include "MakeSharedCheck.h"

 class ModernizeModule : public ClangTidyModule {
 public:
   void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {
+    CheckFactories.registerCheck<AvoidBindCheck>(
+        "modernize-avoid-bind");
     CheckFactories.registerCheck<DeprecatedHeadersCheck>(
         "modernize-deprecated-headers");
     CheckFactories.registerCheck<LoopConvertCheck>("modernize-loop-convert");

docs/clang-tidy/checks/list.rst

    misc-unused-raii
    misc-unused-using-decls
    misc-virtual-near-miss
+   modernize-avoid-bind
    modernize-deprecated-headers
    modernize-loop-convert
    modernize-make-shared

docs/clang-tidy/checks/modernize-avoid-bind.rst

+.. title:: clang-tidy - modernize-avoid-std-bind
+
+modernize-avoid-bind
+==========================
+
+The check finds uses of ``std::bind`` and replaces simple uses with lambdas.
+Lambdas will use value-capture where required.
+
+Right now it only handles free functions, not member functions.
+
+Given:
+
+.. code:: C++
+
+  int add(int x, int y) { return x + y; }
+
+Then:
+
+.. code:: C++
+  
+  void f() {
+    int x = 2;
+    auto clj = std::bind(add, x, _1);
+  }
+
+is replaced by:
+  
+.. code:: C++
+  
+  void f() {
+    int x = 2;
+    auto clj = [=](auto && arg1) { return add(x, arg1); };
+  }
+
+``std::bind`` can be hard to read and can result in larger object files and
+binaries due to type information that will not be produced by equivalent
+lambdas.
+

test/clang-tidy/modernize-avoid-bind.cpp

+// RUN: %check_clang_tidy %s modernize-avoid-bind %t -- -- -std=c++14
+
+namespace std {
+inline namespace impl {
+template <class Fp, class... Arguments>
+class bind_rt {};
+
+template <class Fp, class... Arguments>
+bind_rt<Fp, Arguments...> bind(Fp &&, Arguments &&...);
+}
+}
+
+int add(int x, int y) { return x + y; }
+
+void f() {
+  auto clj = std::bind(add, 2, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind [modernize-avoid-bind]
+  // CHECK-FIXES: auto clj = [] { return add(2, 2); };
+}
+
+void g() {
+  int x = 2;
+  int y = 2;
+  auto clj = std::bind(add, x, y);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind 
+  // CHECK-FIXES: auto clj = [=] { return add(x, y); };
+}
+
+struct placeholder {};
+placeholder _1;
+placeholder _2;
+
+void h() {
+  int x = 2;
+  auto clj = std::bind(add, x, _1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind 
+  // CHECK-FIXES: auto clj = [=](auto && arg1) { return add(x, arg1); };
+}
+
+struct A;
+struct B;
+bool ABTest(const A &, const B &);
+
+void i() {
+  auto BATest = std::bind(ABTest, _2, _1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:17: warning: prefer a lambda to std::bind 
+  // CHECK-FIXES: auto BATest = [](auto && arg1, auto && arg2) { return ABTest(arg2, arg1); };
+}
+
+void j() {
+  auto clj = std::bind(add, 2, 2, 2);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind 
+  // No fix is applied for argument mismatches.
+  // CHECK-FIXES: auto clj = std::bind(add, 2, 2, 2);
+}
+
+void k() {
+  auto clj = std::bind(add, _1, _1);
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind 
+  // No fix is applied for reused placeholders.
+  // CHECK-FIXES: auto clj = std::bind(add, _1, _1);
+}
+
+void m() {
+  auto clj = std::bind(add, 1, add(2, 5));
+  // CHECK-MESSAGES: :[[@LINE-1]]:14: warning: prefer a lambda to std::bind 
+  // No fix is applied for nested calls.
+  // CHECK-FIXES: auto clj = std::bind(add, 1, add(2, 5));
+}
+