[Lldb-commits] [PATCH] D110496: [llvm] [ADT] Add a range/iterator-based Split()

[Pavel Labath via Phabricator via lldb-commits]

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