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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added a comment.

In short, better support like this than lack of suport.
Consider adding option for disabling those free standing functions, reason why I'm telling this is that in case of false-positives, it may be easier just to disable part of functionality than disabling entire check or mass adding NOLINTs.



================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:158-159
+                                       callee(cxxMethodDecl(BeginNameMatcher))),
+                     callExpr(argumentCountIs(1),
+                              callee(functionDecl(BeginNameMatcher)))))
           .bind(BeginCallName);
----------------
Now it will also catch calls to single parameter methods...
maybe `unless(cxxMethodDecl())`, after all we want to catch only functions


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:175
+                        callee(cxxMethodDecl(EndNameMatcher))),
+      callExpr(argumentCountIs(1), callee(functionDecl(EndNameMatcher)))));
 
----------------
Now it will also catch calls to single parameter methods...
maybe unless(cxxMethodDecl()), after all we want to catch only functions


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:283
+                                 hasType(RecordWithBeginEnd)))),
+      callExpr(argumentCountIs(1), callee(functionDecl(hasAnyName("size"))))));
 
----------------
Consider exclude single parameter methods.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:871
+        return false;
+    } else if (Nodes.getNodeAs<CallExpr>(EndCallName)) {
+      return true;
----------------
``else return Nodes.getNodeAs<CallExpr>(EndCallName) != 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);
----------------
Also you could do a mix of member begin and free standing end ?
will it be supported or it wont ?


================
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);
----------------
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.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140760