[PATCH] D16962: clang-tidy: avoid std::bind

[Alexander Kornienko via cfe-commits]

alexfh added inline comments.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:37
@@ +36,3 @@
+
+std::vector<BindArgument>
+buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) {
----------------
`SmallVector<>` would be better here, since the number of arguments is usually rather low.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:38
@@ +37,3 @@
+std::vector<BindArgument>
+buildBindArguments(const MatchFinder::MatchResult &Result, const CallExpr *C) {
+  std::vector<BindArgument> BindArguments;
----------------
Please make the functions static.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:42
@@ +41,3 @@
+
+  for (size_t I = 1, ArgCount = C->getNumArgs(); I < ArgCount; ++I) {
+    const Expr *E = C->getArg(I);
----------------
Please use a range-based for loop over `C->arguments()`.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:45
@@ +44,3 @@
+    BindArgument B;
+    if (auto M = dyn_cast<MaterializeTemporaryExpr>(E)) {
+      auto TE = M->GetTemporaryExpr();
----------------
Should be `const auto *M` to make it clear M is a pointer. Same for other code like this.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:67
@@ +66,3 @@
+
+void addPlaceholderArgs(const std::vector<BindArgument> &Args,
+                        llvm::raw_ostream &Stream) {
----------------
Please use `ArrayRef<>` to pass a constant reference to a sequence of elements.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:100
@@ +99,3 @@
+  }
+  Stream << "); };";
+}
----------------
I don't like the asymmetry here: the opening parenthesis is added in the caller and the closing parenthesis is added here. 

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:104
@@ +103,3 @@
+bool isPlaceHolderIndexRepeated(const std::vector<BindArgument> &Args) {
+  std::unordered_map<size_t, size_t> PlaceHolderIndexCounts;
+  for (const BindArgument &B : Args) {
----------------
  llvm::SmallPtrSet<size_t> PlaceHolderIndices;
  for (const BindArgument &B : Args) {
    if (B.PlaceHolderIndex) {
      if (!PlaceHolderIndices.insert(B.PlaceHolderIndex).second)
        return true;
    }
  }
  return false;

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:126
@@ +125,3 @@
+  const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("bind");
+  auto DiagnosticBuilder =
+      diag(MatchedDecl->getLocStart(), "avoid using std::bind");
----------------
Many other checks use just `Diag`.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:127
@@ +126,3 @@
+  auto DiagnosticBuilder =
+      diag(MatchedDecl->getLocStart(), "avoid using std::bind");
+
----------------
Should the message recommend something instead?

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:127
@@ +126,3 @@
+  auto DiagnosticBuilder =
+      diag(MatchedDecl->getLocStart(), "avoid using std::bind");
+
----------------
alexfh wrote:
> Should the message recommend something instead?
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.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:129
@@ +128,3 @@
+
+  if (!getLangOpts().CPlusPlus14) // Need C++14 for generic lambdas
+    return;
----------------
nit: Trailing period.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:139
@@ +138,3 @@
+  // sharing between outer and inner std:bind invocations.
+  if (std::find_if(Args.begin(), Args.end(), [](const BindArgument &B) {
+        return B.Kind == BK_CallExpr;
----------------
This would be shorter with `std::any_of` or even better with `llvm::any_of` that accepts a range instead of begin/end.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.cpp:163
@@ +162,3 @@
+  bool HasCapturedArgument =
+      std::find_if(Args.begin(), Args.end(), [](const BindArgument &B) {
+        return B.Kind == BK_Other;
----------------
Use `llvm::any_of`.

================
Comment at: clang-tidy/readability/AvoidStdBindCheck.h:1
@@ +1,2 @@
+//===--- AvoidStdBindCheck.h - clang-tidy-----------------------------*- C++
+//-*-===//
----------------
Join with the next line and remove extra dashes, please.

================
Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:4
@@ +3,3 @@
+readability-avoid-std-bind
+===================
+
----------------
nit: Add moar ekwality.

================
Comment at: docs/clang-tidy/checks/readability-avoid-std-bind.rst:6
@@ +5,3 @@
+
+Find uses of std::bind. Replace simple uses of std::bind with lambdas. Lambdas
+will use value-capture where required.
----------------
Use double backquotes to highlight inline code snippets like `std::bind`.


http://reviews.llvm.org/D16962