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

[Piotr Zegar via Phabricator via cfe-commits]

PiotrZSL added inline comments.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:74
+                                                "rend",    "crend",  "size"};
+static const std::set<llvm::StringRef> StdNames{
+    "std::begin", "std::cbegin", "std::rbegin", "std::crbegin", "std::end",
----------------
consider: #include "llvm/ADT/StringSet.h" & llvm::StringSet


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:302
+                                 hasType(RecordWithBeginEnd)))),
+      callExpr(argumentCountIs(1), callee(functionDecl(hasAnyName("size"))),
+               usesADL()),
----------------
hasAnyName -> hasName, there is only 1 argument.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:305
+      callExpr(argumentCountIs(1),
+               callee(functionDecl(hasAnyName("::std::size"))))));
 
----------------
hasAnyName -> hasName, there is only 1 argument.


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:346
+// The returned Expr* is nullptr if any of the assumptions are not met.
+static std::tuple<const Expr *, StringRef, bool, IteratorCallKind>
+getContainerExpr(const Expr *Call) {
----------------
change this tuple into some struct, it's hard to find out what those arguments are...


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:354
+    CallKind = ICK_Member;
+    if (const auto *Member = dyn_cast<MemberExpr>(TheCall->getCallee())) {
+      return std::make_tuple(TheCall->getImplicitObjectArgument(),
----------------
STYLE: no need for `if (xx) { y1 } else { y2 }`, just `if (xx) y1 else y2;` should be sufficient


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:363
+    }
+  } else if (const auto *TheCall = dyn_cast_or_null<CallExpr>(Dug)) {
+    if (TheCall->getNumArgs() != 1)
----------------
STYLE: this else is not needed, there are returns in previous anyway
simply if will be sufficient


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:368
+    if (TheCall->usesADL()) {
+      if (!ADLNames.count(TheCall->getDirectCallee()->getName()))
+        return {};
----------------
count -> contains


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:372
+    } else {
+      if (!StdNames.count(
+              TheCall->getDirectCallee()->getQualifiedNameAsString()))
----------------
count -> contains


================
Comment at: clang-tools-extra/clang-tidy/modernize/LoopConvertCheck.cpp:390
 /// is called via . or ->.
-static const Expr *getContainerFromBeginEndCall(const Expr *Init, bool IsBegin,
-                                                bool *IsArrow, bool IsReverse) {
+static std::tuple<const Expr *, IteratorCallKind>
+getContainerFromBeginEndCall(const Expr *Init, bool IsBegin, bool *IsArrow,
----------------
no need for tuple for 2 argments, pair would be sufficient.


================
Comment at: clang-tools-extra/docs/ReleaseNotes.rst:201-204
+- Improved :doc:`modernize-loop-convert
+  <clang-tidy/checks/modernize/loop-convert>` to refactor container based
+  for loops that initialize iterators with free function calls to ``begin``,
+  ``end``, or ``size``.
----------------
maybe just "Enhanced XYZ check to support for-loops with iterators initialized by free function calls like begin, end, or size."
I got lost on this part "to refactor container based for loops"


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp:477
+
+  for (S::const_iterator It = cbegin(Ss), E = cend(Ss); It != E; ++It) {
+    printf("s4 has value %d\n", (*It).X);
----------------
what if begin,end is missing, and there is only cbegin, cend then it will fail with:
"error: invalid range expression of type 'const A'; no viable 'begin' function available|



================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp:481
+  // CHECK-MESSAGES: :[[@LINE-3]]:3: warning: use range-based for loop instead
+  // CHECK-FIXES: for (auto It : Ss)
+  // CHECK-FIXES-NEXT: printf("s4 has value %d\n", It.X);
----------------
shouldnt it be `const& It` ?


================
Comment at: clang-tools-extra/test/clang-tidy/checkers/modernize/loop-convert-basic.cpp:737
+  // CHECK-FIXES: for (auto & Val : Vals)
+  // CHECK-FIXES-NEXT: Sum += Val.X;
 }
----------------
add some tests for C++20 with rbegin, rend


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140760