[Lldb-commits] [PATCH] D110535: [llvm] [ADT] Update llvm::Split() per Pavel Labath's suggestions

[Michał Górny via Phabricator via lldb-commits]

mgorny marked an inline comment as done.
mgorny added inline comments.


================
Comment at: llvm/include/llvm/ADT/StringExtras.h:544
 /// of the iterators.
-class Split {
-  StringRef Str;
-  std::string SeparatorStr;
-
-public:
-  Split(StringRef NewStr, StringRef Separator)
-      : Str(NewStr), SeparatorStr(Separator) {}
-  Split(StringRef NewStr, char Separator)
-      : Str(NewStr), SeparatorStr(1, Separator) {}
-
-  SplittingIterator begin() { return SplittingIterator(Str, SeparatorStr); }
-
-  SplittingIterator end() { return SplittingIterator("", SeparatorStr); }
-};
+inline iterator_range<SplittingIterator> Split(StringRef Str, StringRef Separator) {
+  return {SplittingIterator(Str, Separator),
----------------
labath wrote:
> this should also be a lower-case `split` now that it's a function.
> 
> (Believe it or not, this is the reason I was first drawn to this -- it's uncommon to see a class used like this because, even in the cases where you do have a separate class, you usually create a function wrapper for it. The original reason for this is pragmatic (use of template argument deduction), but the practice is so ubiquitous that a deviation from it stands out.)
Yeah, that makes sense. In fact, I originally named the class lowercase for this exact reason.


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

https://reviews.llvm.org/D110535