[Lldb-commits] [PATCH] D110535: [llvm] [ADT] Update llvm::Split() per Pavel Labath's suggestions
Michał Górny via Phabricator via lldb-commits
lldb-commits at lists.llvm.org
Mon Sep 27 06:16:42 PDT 2021
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
More information about the lldb-commits
mailing list