[PATCH] D97955: [clang-tidy] Refactor loop-convert to bring most of the checking into matchers

[Stephen Kelly via Phabricator via cfe-commits]

steveire added a comment.

Good progress! This check also implements its own `StmtAncestorASTVisitor` which should probably be removed in favor of the `ParentMapContext` eventually (not in this patch).



================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:118
+
+AST_MATCHER(ParmVarDecl, hasDefaultArg) {
+  return Node.getDefaultArg() != nullptr;
----------------
There's already a (deprecated) 	`hasDefaultArgument` in `ASTMatchers.h` with the same implementation. 

It is deprecated in favor of `hasInitializer`.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:152
+  struct Predicate {
+    bool operator()(const ast_matchers::internal::BoundNodesMap &Nodes) const {
+      const auto *BeginMember =
----------------
Why not use lambdas?


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:236
 
-  DeclarationMatcher EndDeclMatcher =
-      varDecl(hasInitializer(anything())).bind(EndVarName);
-
-  StatementMatcher EndCallMatcher = cxxMemberCallExpr(
-      argumentCountIs(0), callee(cxxMethodDecl(EndNameMatcher)));
+  StatementMatcher EndCallMatcher =
+      cxxMemberCallExpr(argumentCountIs(0),
----------------
It always makes sense to use `auto` for matchers. Here, you hide an unnecessary implicit conversion from `BindableMatcher<Stmt>` to `Matcher<Stmt>`. 

Below you use `auto` for a matcher. Why be inconsistent?


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:859
   const auto *LoopVar = Nodes.getNodeAs<VarDecl>(InitVarName);
-  const auto *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
-
-  // If the loop calls end()/size() after each iteration, lower our confidence
-  // level.
-  if (FixerKind != LFK_Array && !EndVar)
-    ConfidenceLevel.lowerTo(Confidence::CL_Reasonable);
+  const VarDecl *EndVar = Nodes.getNodeAs<VarDecl>(EndVarName);
 
----------------
Why change `auto` to `VarDecl` (repeated) here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97955