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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
