ccotter marked 3 inline comments as done.
ccotter added inline comments.
================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:175
+ callee(cxxMethodDecl(EndNameMatcher))),
+ callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcher)))));
----------------
PiotrZSL wrote:
> Now it will also catch calls to single parameter methods...
> maybe unless(cxxMethodDecl()), after all we want to catch only functions
Updated this to properly only look at 1) a qualified call to `std::begin` etc,
or 2) an ADL call to `begin` etc.
================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:871
+ return false;
+ } else if (Nodes.getNodeAs<CallExpr>(EndCallName)) {
+ return true;
----------------
PiotrZSL wrote:
> ``else return Nodes.getNodeAs<CallExpr>(EndCallName) != nullptr;``
Is this in the style guide? NotNullTerminatedResultCheck.cpp,
SignedCharMisuseCheck.cpp, UnconventionalAssignOperatorCheck.cpp have
equivalent conditionals that do not check against `nullptr`.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp:449
+
+ for (S::iterator It = begin(Ss), E = end(Ss); It != E; ++It) {
+ printf("s has value %d\n", (*It).X);
----------------
PiotrZSL wrote:
> Also you could do a mix of member begin and free standing end ?
> will it be supported or it wont ?
No, I updated the logic and added test cases. I don't think mixing makes sense
since range based loops look at symmetric member methods or ADL lookups but not
mixed cases. This would not be a common case anyway in code I expect.
================
Comment at:
clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp:456
+
+ for (S::iterator It = begin(*Ps), E = end(*Ps); It != E; ++It) {
+ printf("s has value %d\n", (*It).X);
----------------
PiotrZSL wrote:
> would be nice to validate if begin/end got same argument, but for that
> probably we would need to extract isIdenticalStmt function from
> clang/lib/StaticAnalyzer/Checkers/IdenticalExprChecker.cpp into separate
> matcher, so thats not a topic for this change.
>
The existing logic handles this. I'll add a negative test case.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140760/new/
https://reviews.llvm.org/D140760
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
