[PATCH] D70368: [clang-tidy] Rewrite modernize-avoid-bind check

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is a great re-write, thank you for working on it!



================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:105
+  CallableInfo Callable;
+
+  SmallVector<BindArgument, 4> BindArguments;
----------------
You can remove some newlines here.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:116
+static const Expr *ignoreTemporariesAndImplicitCasts(const Expr *E) {
+  if (const auto *T = dyn_cast<MaterializeTemporaryExpr>(E))
+    return ignoreTemporariesAndImplicitCasts(T->GetTemporaryExpr());
----------------
What about `CXXBindTemporaryExpr`?

Would `Expr::IgnoreImplicit()` do too much stripping because it removes `FullExpr` as well?


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:124
+static const Expr *ignoreTemporariesAndPointers(const Expr *E) {
+
+  if (const auto *T = dyn_cast<UnaryOperator>(E))
----------------
Spurious newline


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:130
+
+  if (const auto *T = dyn_cast<MaterializeTemporaryExpr>(E))
+    return ignoreTemporariesAndPointers(T->GetTemporaryExpr());
----------------
Similar question here about bound temporaries as above.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:143-147
+  std::string S;
+  llvm::raw_string_ostream OS(S);
+  OS << "capture" << CaptureIndex++;
+  OS.flush();
+  return S;
----------------
`llvm::utostr()` instead of using a streaming interface?


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:144
+  std::string S;
+  llvm::raw_string_ostream OS(S);
+  OS << "capture" << CaptureIndex++;
----------------
I think you can drop the `llvm::` here and elsewhere in the file.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:153
+
+  return HNM.matchesNode(*dyn_cast<NamedDecl>(D));
+}
----------------
This should probably use `cast<>` if it's going to assume the returned value is never null.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:196-199
+  for (const auto *Child : Statement->children()) {
+    if (anyDescendantIsLocal(Child))
+      return true;
+  }
----------------
`return llvm::any_of(Statement->children(), anyDescendantIsLocal);`?


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:431-434
+  if (!Candidates.empty())
+    return Candidates;
+
+  return Candidates;
----------------
It seems like `return Candidates;` will suffice. :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70368/new/

https://reviews.llvm.org/D70368