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