[PATCH] D140760: [clang-tidy] Support begin/end free functions in modernize-loop-convert

[Chris Cotter via Phabricator via cfe-commits]

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