[PATCH] D81769: [clang-tidy] Repair various issues with modernize-avoid-bind

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D81769#2106574 <https://reviews.llvm.org/D81769#2106574>, @jaafar wrote:

> "Ping". I hope this can be considered for 10.0.1. If nothing else I think reviewing the test cases has a lot of value - there are some real issues with the current checks and fixits.


Thank you for all of these fixes! I like the direction it's going in, but am a bit wary of introducing this for 10.0.1 because it changes a lot of behaviors (not to say it shouldn't be 10.0.1, just to express a bit of trepidation).



================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:150
 
+static bool tryCaptureAsLocalVariable(const MatchFinder::MatchResult &Result,
+                                      BindArgument &B, const Expr *E);
----------------
jaafar wrote:
> These forward declarations seemed to make the diffs a lot easier to read. It's also possible to move the two functions into this position, of course.
I think it's fine to use forward declares, but I'd prefer them just after the anonymous namespace (or before it) so the declarations are towards the top of the file instead of stuck in the middle.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:166
+    } else {
+      // the argument to std::ref is an expression that produces a reference
+      // create a capture reference to hold it
----------------
Comments should be grammatical, so include capitalization and punctuation. (Here and elsewhere in the patch.)


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:243
+  if ((std::distance(ME->child_begin(), ME->child_end()) == 1) &&
+      isa<CXXThisExpr>(*ME->children().begin())) {
+    // reference to data member without explicit "this"
----------------
jaafar wrote:
> Is there a better way to express this? "a single child of type ThisExpr" was what I was going for...
`if (isa<CXXThisExpr>(ME->getBase()))` -- `MemberExpr::children()` only considers the base anyway, so there's only ever one child node to begin with.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:282
     SmallVector<StringRef, 2> Matches;
-    if (MatchPlaceholder.match(B.SourceTokens, &Matches)) {
+    clang::DeclRefExpr const *DRE = dyn_cast<DeclRefExpr>(E);
+    if (MatchPlaceholder.match(B.SourceTokens, &Matches) ||
----------------
`const auto *` since the type is spelled out in the initialization.


================
Comment at: clang-tools-extra/clang-tidy/modernize/AvoidBindCheck.cpp:414-418
+    if (D->getKind() != Decl::Kind::FunctionTemplate)
+      continue;
+
+    const clang::FunctionTemplateDecl *FT =
+        clang::dyn_cast<FunctionTemplateDecl>(D);
----------------
This doesn't need to be done in two steps. Better:
```
const auto *FTD = dyn_cast<FunctionTemplateDecl>(D);
if (!FTD)
  continue;

const FunctionDecl *FD = FTD->getTemplatedDecl();
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81769