labath added a comment.
I like it as well :)
================
Comment at: llvm/include/llvm/ADT/StringExtras.h:519
+ bool operator==(const SplittingIterator &R) const {
+ return Current == R.Current && Next == R.Next && Separator == R.Separator;
+ }
----------------
This compares the contents of the StringRefs, while it should be comparing the
pointers instead. I don't think it can cause correctness issues though I think
it might be able to cause super-linear iteration complexity for some
pathological inputs and more complex algorithms (a string consisting of many
identical substrings, and an algorithm which compares two successive iterators).
Since comparing iterators from different sequences is UB, I think that
comparing just `Current.data()` could actually suffice (the rest could be
asserts) -- one has to be careful how he initializes the past-the-end iterator
though.
================
Comment at: llvm/include/llvm/ADT/StringExtras.h:527-529
+ std::pair<StringRef, StringRef> Res = Next.split(Separator);
+ Current = Res.first;
+ Next = Res.second;
----------------
`std::tie(Current, Next) = ...`
================
Comment at: llvm/include/llvm/ADT/StringExtras.h:545-558
+class Split {
+ StringRef Str;
+ std::string SeparatorStr;
+
+public:
+ Split(StringRef NewStr, StringRef Separator)
+ : Str(NewStr), SeparatorStr(Separator) {}
----------------
Could this be a function that returns `iterator_range<SplittingIterator>` ?
(I suppose lifetimes could be an issue, but it's not clear to me if that's the
case here)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D110496/new/
https://reviews.llvm.org/D110496
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
