[llvm] [ADT] Update llvm::Split() per Pavel Labath's suggestions

[mgorny]

Summary

Optimize the iterator comparison logic to compare Current.data() pointers. Use std::tie for assignments from std::pair. Replace the custom class with a function returning iterator_range.

[labath]

llvm/include/llvm/ADT/StringExtras.h

576–579

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.)

[mgorny]

llvm/include/llvm/ADT/StringExtras.h

576–579

Yeah, that makes sense. In fact, I originally named the class lowercase for this exact reason.

[mgorny]

Make the function lowercase. I had to modify IR/DataLayout.cpp to call its own static split() via ::split().

[joerg]

Why are all the changes from separator character to separator string necessary or desirable?

[mgorny]

In D110535#3028450, @joerg wrote:

Why are all the changes from separator character to separator string necessary or desirable?

Because otherwise we'd have to resort to hacks to extend the separator's lifetime.

[labath]

Looks good to me, but I'd give @joerg a chance to respond first.

In D110535#3028450, @joerg wrote:

Why are all the changes from separator character to separator string necessary or desirable?

There's no good place to store the separator character. The functional version of split is implemented by creating a StringRef(&the_char, 1), but that won't work in the iterator version. Keeping it doesn't seem to be worth the trouble, as it does not have any performance benefits, and doesn't make the callsites more complicated.

[joerg]

Can't you wrap iterator_range<T> and possibly even support Twines like that? That is, don't extend the life time of the iterators, but store it in the range?

[mgorny]

In D110535#3047033, @joerg wrote:

Can't you wrap iterator_range<T> and possibly even support Twines like that? That is, don't extend the life time of the iterators, but store it in the range?

I can but @labath didn't want the extra code.

[labath]

In D110535#3047262, @mgorny wrote:

In D110535#3047033, @joerg wrote:

Can't you wrap iterator_range<T> and possibly even support Twines like that? That is, don't extend the life time of the iterators, but store it in the range?

I can but @labath didn't want the extra code.

I am not sure that it is worth it, but I am not completely opposed to the idea either. Symmetry with StringRef::split also counts for something..

[mgorny]

So how should I proceed?

[lattner]

Do you have commit access? If not, please read the llvm developer policy and follow the instructions, thanks!

[mgorny]

In D110535#3075563, @lattner wrote:

Do you have commit access? If not, please read the llvm developer policy and follow the instructions, thanks!

That's not the problem. The problem is that @labath and @joerg seem not to agree on how it should be done, and I don't know whether to hack support for char delimiter to preserve the old syntax as @joerg wanted, or to keep it as is (i.e. without support for char delimiter) as @labath originally suggested.

[labath]

I'm fine with either option, and it doesn't seem to me like @joerg has a particularly strong more opinion either, so maybe you can cast the deciding vote?

I consider it more important to fix the equality operator and the name capitalization (in that order).

[mgorny]

Restored support for char Separator — after all, it isn't hard and the cost is negligible.

[mgorny]

Update LLDB call sites again.

[mgorny]

Add explicit test for use in a for loop.

[mgorny]

Something's broken.

[mgorny]

Ok, this turned out to be a little bit harder than I anticipated — in particular I needed to add a copy constructor and assignment operator.

@labath, do you think it's fine or should I revert to non-char version?

[labath]

It makes things a bit more complicated, which then makes the case for dropping char slightly stronger, but I still don't think it's worth making a fuss of it.

llvm/unittests/ADT/StringExtrasTest.cpp

325

maybe EXPECT_THAT(Result, testing::ElementsAre("foo", "bar", "", "baz"));

lldb/source/Host/common/File.cpp

else if (open_options & eOpenOptionCanCreate)
     mode |= O_CREAT;
 
   return mode;
 }
 
 llvm::Expected<SerialPort::Options>
 SerialPort::OptionsFromURL(llvm::StringRef urlqs) {
   SerialPort::Options serial_options;
-  for (llvm::StringRef x : llvm::Split(urlqs, '&')) {
+  for (llvm::StringRef x : llvm::split(urlqs, '&')) {
     if (x.consume_front("baud=")) {
       unsigned int baud_rate;
       if (!llvm::to_integer(x, baud_rate, 10))
         return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                        "Invalid baud rate: %s",
                                        x.str().c_str());
       serial_options.BaudRate = baud_rate;
     } else if (x.consume_front("parity=")) {

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp

void GDBRemoteCommunicationClient::GetRemoteQSupported() {
 
   StringExtractorGDBRemote response;
   if (SendPacketAndWaitForResponse(packet.GetString(), response) ==
       PacketResult::Success) {
     // Hang on to the qSupported packet, so that platforms can do custom
     // configuration of the transport before attaching/launching the process.
     m_qSupported_response = response.GetStringRef().str();
 
-    for (llvm::StringRef x : llvm::Split(response.GetStringRef(), ';')) {
+    for (llvm::StringRef x : llvm::split(response.GetStringRef(), ';')) {
       if (x == "qXfer:auxv:read+")
         m_supports_qXfer_auxv_read = eLazyBoolYes;
       else if (x == "qXfer:libraries-svr4:read+")
         m_supports_qXfer_libraries_svr4_read = eLazyBoolYes;
       else if (x == "augmented-libraries-svr4-read") {
         m_supports_qXfer_libraries_svr4_read = eLazyBoolYes; // implied
         m_supports_augmented_libraries_svr4_read = eLazyBoolYes;
       } else if (x == "qXfer:libraries:read+")
if (SendPacketAndWaitForResponse(packet, response) ==
         } else if (name.equals("error")) {
           StringExtractorGDBRemote error_extractor(value);
           std::string error_string;
           // Now convert the HEX bytes into a string value
           error_extractor.GetHexByteString(error_string);
           error.SetErrorString(error_string.c_str());
         } else if (name.equals("dirty-pages")) {
           std::vector<addr_t> dirty_page_list;
-          for (llvm::StringRef x : llvm::Split(value, ',')) {
+          for (llvm::StringRef x : llvm::split(value, ',')) {
             addr_t page;
             x.consume_front("0x");
             if (llvm::to_integer(x, page, 16))
               dirty_page_list.push_back(page);
           }
           region_info.SetDirtyPageList(dirty_page_list);
         }
       }

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp

if (!m_current_process ||
       (m_current_process->GetID() == LLDB_INVALID_PROCESS_ID))
     return SendErrorResponse(Status("Process not running."));
 
   std::string path_hint;
 
   StringRef packet_str{packet.GetStringRef()};
   assert(packet_str.startswith("qSaveCore"));
   if (packet_str.consume_front("qSaveCore;")) {
-    for (auto x : llvm::Split(packet_str, ';')) {
+    for (auto x : llvm::split(packet_str, ';')) {
       if (x.consume_front("path-hint:"))
         StringExtractor(x).GetHexByteString(path_hint);
       else
         return SendErrorResponse(Status("Unsupported qSaveCore option"));
     }
   }
 
   llvm::Expected<std::string> ret = m_current_process->SaveCore(path_hint);

lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp

bool ProcessGDBRemote::ParsePythonTargetDefinition(
   }
   return false;
 }
 
 static size_t SplitCommaSeparatedRegisterNumberString(
     const llvm::StringRef &comma_separated_register_numbers,
     std::vector<uint32_t> &regnums, int base) {
   regnums.clear();
-  for (llvm::StringRef x : llvm::Split(comma_separated_register_numbers, ',')) {
+  for (llvm::StringRef x : llvm::split(comma_separated_register_numbers, ',')) {
     uint32_t reg;
     if (llvm::to_integer(x, reg, base))
       regnums.push_back(reg);
   }
   return regnums.size();
 }
 
 void ProcessGDBRemote::BuildDynamicRegisterInfo(bool force) {
size_t ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(
   } while (thread_ids.GetChar() == ',');
 
   return m_thread_ids.size();
 }
 
 size_t ProcessGDBRemote::UpdateThreadPCsFromStopReplyThreadsValue(
     llvm::StringRef value) {
   m_thread_pcs.clear();
-  for (llvm::StringRef x : llvm::Split(value, ',')) {
+  for (llvm::StringRef x : llvm::split(value, ',')) {
     lldb::addr_t pc;
     if (llvm::to_integer(x, pc, 16))
       m_thread_pcs.push_back(pc);
   }
   return m_thread_pcs.size();
 }
 
 bool ProcessGDBRemote::UpdateThreadIDList() {
if (m_gdb_comm.SendPacketAndWaitForResponse(packet.GetString(), response) ==
     if (response.IsErrorResponse())
       return llvm::createStringError(
           llvm::inconvertibleErrorCode(),
           llvm::formatv("qSaveCore returned an error"));
 
     std::string path;
 
     // process the response
-    for (auto x : llvm::Split(response.GetStringRef(), ';')) {
+    for (auto x : llvm::split(response.GetStringRef(), ';')) {
       if (x.consume_front("core-path:"))
         StringExtractor(x).GetHexByteString(path);
     }
 
     // verify that we've gotten what we need
     if (path.empty())
       return llvm::createStringError(llvm::inconvertibleErrorCode(),
                                      "qSaveCore returned no core path");

llvm/include/llvm/ADT/StringExtras.h

operator StringRef() {
     return Separator;
   }
 };
 
 /// A forward iterator over partitions of string over a separator.
 class SplittingIterator
     : public iterator_facade_base<SplittingIterator, std::forward_iterator_tag,
                                   StringRef> {
+  char SeparatorStorage;
   StringRef Current;
   StringRef Next;
   StringRef Separator;
 
 public:
   SplittingIterator(StringRef Str, StringRef Separator)
       : Next(Str), Separator(Separator) {
     ++*this;
   }
 
+  SplittingIterator(StringRef Str, char Separator)
+      : SeparatorStorage(Separator), Next(Str),
+        Separator(&SeparatorStorage, 1) {
+    ++*this;
+  }
+
+  SplittingIterator(const SplittingIterator &R)
+      : SeparatorStorage(R.SeparatorStorage), Current(R.Current), Next(R.Next),
+        Separator(R.Separator) {
+    if (R.Separator.data() == &R.SeparatorStorage)
+      Separator = StringRef(&SeparatorStorage, 1);
+  }
+
+  SplittingIterator &operator=(const SplittingIterator &R) {
+    if (this == &R)
+      return *this;
+
+    SeparatorStorage = R.SeparatorStorage;
+    Current = R.Current;
+    Next = R.Next;
+    Separator = R.Separator;
+    if (R.Separator.data() == &R.SeparatorStorage)
+      Separator = StringRef(&SeparatorStorage, 1);
+    return *this;
+  }
+
   bool operator==(const SplittingIterator &R) const {
-    return Current == R.Current && Next == R.Next && Separator == R.Separator;
+    assert(Separator == R.Separator);
+    return Current.data() == R.Current.data();
   }
 
   const StringRef &operator*() const { return Current; }
 
   StringRef &operator*() { return Current; }
 
   SplittingIterator &operator++() {
-    std::pair<StringRef, StringRef> Res = Next.split(Separator);
-    Current = Res.first;
-    Next = Res.second;
+    std::tie(Current, Next) = Next.split(Separator);
     return *this;
   }
 };
 
 /// Split the specified string over a separator and return a range-compatible
 /// iterable over its partitions.  Used to permit conveniently iterating
 /// over separated strings like so:
 ///
 /// \code
-///   for (StringRef x : llvm::Split("foo,bar,baz", ','))
+///   for (StringRef x : llvm::split("foo,bar,baz", ","))
 ///     ...;
 /// \end
 ///
 /// Note that the passed string must remain valid throuhgout lifetime
 /// 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); }
+inline iterator_range<SplittingIterator> split(StringRef Str, StringRef Separator) {
+  return {SplittingIterator(Str, Separator),
+          SplittingIterator(StringRef(), Separator)};
+}
 
-  SplittingIterator end() { return SplittingIterator("", SeparatorStr); }
-};
+inline iterator_range<SplittingIterator> split(StringRef Str, char Separator) {
+  return {SplittingIterator(Str, Separator),
+          SplittingIterator(StringRef(), Separator)};
+}
 
 } // end namespace llvm
 
 #endif // LLVM_ADT_STRINGEXTRAS_H

llvm/lib/IR/DataLayout.cpp

static Error getAddrSpace(StringRef R, unsigned &AddrSpace) {
   return Error::success();
 }
 
 Error DataLayout::parseSpecifier(StringRef Desc) {
   StringRepresentation = std::string(Desc);
   while (!Desc.empty()) {
     // Split at '-'.
     std::pair<StringRef, StringRef> Split;
-    if (Error Err = split(Desc, '-', Split))
+    if (Error Err = ::split(Desc, '-', Split))
       return Err;
     Desc = Split.second;
 
     // Split at ':'.
-    if (Error Err = split(Split.first, ':', Split))
+    if (Error Err = ::split(Split.first, ':', Split))
       return Err;
 
     // Aliases used below.
     StringRef &Tok  = Split.first;  // Current token.
     StringRef &Rest = Split.second; // The rest of the string.
 
     if (Tok == "ni") {
       do {
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
         Rest = Split.second;
         unsigned AS;
         if (Error Err = getInt(Split.first, AS))
           return Err;
         if (AS == 0)
           return reportError("Address space 0 can never be non-integral");
         NonIntegralAddressSpaces.push_back(AS);
case 'p': {
           return Err;
       if (!isUInt<24>(AddrSpace))
         return reportError("Invalid address space, must be a 24bit integer");
 
       // Size.
       if (Rest.empty())
         return reportError(
             "Missing size specification for pointer in datalayout string");
-      if (Error Err = split(Rest, ':', Split))
+      if (Error Err = ::split(Rest, ':', Split))
         return Err;
       unsigned PointerMemSize;
       if (Error Err = getIntInBytes(Tok, PointerMemSize))
         return Err;
       if (!PointerMemSize)
         return reportError("Invalid pointer size of 0 bytes");
 
       // ABI alignment.
       if (Rest.empty())
         return reportError(
             "Missing alignment specification for pointer in datalayout string");
-      if (Error Err = split(Rest, ':', Split))
+      if (Error Err = ::split(Rest, ':', Split))
         return Err;
       unsigned PointerABIAlign;
       if (Error Err = getIntInBytes(Tok, PointerABIAlign))
         return Err;
       if (!isPowerOf2_64(PointerABIAlign))
         return reportError("Pointer ABI alignment must be a power of 2");
 
       // Size of index used in GEP for address calculation.
       // The parameter is optional. By default it is equal to size of pointer.
       unsigned IndexSize = PointerMemSize;
 
       // Preferred alignment.
       unsigned PointerPrefAlign = PointerABIAlign;
       if (!Rest.empty()) {
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
         if (Error Err = getIntInBytes(Tok, PointerPrefAlign))
           return Err;
         if (!isPowerOf2_64(PointerPrefAlign))
           return reportError(
               "Pointer preferred alignment must be a power of 2");
 
         // Now read the index. It is the second optional parameter here.
         if (!Rest.empty()) {
-          if (Error Err = split(Rest, ':', Split))
+          if (Error Err = ::split(Rest, ':', Split))
             return Err;
           if (Error Err = getIntInBytes(Tok, IndexSize))
             return Err;
           if (!IndexSize)
             return reportError("Invalid index size of 0 bytes");
         }
       }
       if (Error Err = setPointerAlignment(
case 'a': {
       if (AlignType == AGGREGATE_ALIGN && Size != 0)
         return reportError(
             "Sized aggregate specification in datalayout string");
 
       // ABI alignment.
       if (Rest.empty())
         return reportError(
             "Missing alignment specification in datalayout string");
-      if (Error Err = split(Rest, ':', Split))
+      if (Error Err = ::split(Rest, ':', Split))
         return Err;
       unsigned ABIAlign;
       if (Error Err = getIntInBytes(Tok, ABIAlign))
         return Err;
       if (AlignType != AGGREGATE_ALIGN && !ABIAlign)
         return reportError(
             "ABI alignment specification must be >0 for non-aggregate types");
 
       if (!isUInt<16>(ABIAlign))
         return reportError("Invalid ABI alignment, must be a 16bit integer");
       if (ABIAlign != 0 && !isPowerOf2_64(ABIAlign))
         return reportError("Invalid ABI alignment, must be a power of 2");
 
       // Preferred alignment.
       unsigned PrefAlign = ABIAlign;
       if (!Rest.empty()) {
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
         if (Error Err = getIntInBytes(Tok, PrefAlign))
           return Err;
       }
 
       if (!isUInt<16>(PrefAlign))
         return reportError(
             "Invalid preferred alignment, must be a 16bit integer");
case 'n':  // Native integer types.
         if (Error Err = getInt(Tok, Width))
           return Err;
         if (Width == 0)
           return reportError(
               "Zero width native integer type in datalayout string");
         LegalIntWidths.push_back(Width);
         if (Rest.empty())
           break;
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
       }
       break;
     case 'S': { // Stack natural alignment.
       uint64_t Alignment;
       if (Error Err = getIntInBytes(Tok, Alignment))
         return Err;
       if (Alignment != 0 && !llvm::isPowerOf2_64(Alignment))

llvm/unittests/ADT/StringExtrasTest.cpp

 //===- StringExtrasTest.cpp - Unit tests for String extras ----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
 
 TEST(StringExtrasTest, isPrint) {
   EXPECT_FALSE(isPrint('\0'));
   EXPECT_FALSE(isPrint('\t'));
   EXPECT_TRUE(isPrint('0'));
TEST(StringExtrasTest, toStringAPSInt) {
   isUnsigned = false;
   EXPECT_EQ(toString(APSInt(APInt(8, 255), isUnsigned), 2), "-1");
   EXPECT_EQ(toString(APSInt(APInt(8, 255), isUnsigned), 8), "-1");
   EXPECT_EQ(toString(APSInt(APInt(8, 255), isUnsigned), 10), "-1");
   EXPECT_EQ(toString(APSInt(APInt(8, 255), isUnsigned), 16), "-1");
 }
 
 TEST(StringExtrasTest, splitStringRef) {
-  auto Spl = Split("foo<=>bar<=><=>baz", "<=>");
+  auto Spl = split("foo<=>bar<=><=>baz", "<=>");
   auto It = Spl.begin();
   auto End = Spl.end();
 
   ASSERT_NE(It, End);
   EXPECT_EQ(*It, StringRef("foo"));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef("bar"));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef(""));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef("baz"));
   ASSERT_EQ(++It, End);
 }
 
-TEST(StringExtrasTest, splItChar) {
-  auto Spl = Split("foo,bar,,baz", ',');
+TEST(StringExtrasTest, splitStringRefForLoop) {
+  llvm::SmallVector<StringRef, 4> Result;
+  for (StringRef x : split("foo<=>bar<=><=>baz", "<=>"))
+    Result.push_back(x);
+  EXPECT_THAT(Result, testing::ElementsAre("foo", "bar", "", "baz"));
+}
+
+TEST(StringExtrasTest, splitChar) {
+  auto Spl = split("foo,bar,,baz", ',');
   auto It = Spl.begin();
   auto End = Spl.end();
 
   ASSERT_NE(It, End);
   EXPECT_EQ(*It, StringRef("foo"));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef("bar"));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef(""));
   ASSERT_NE(++It, End);
   EXPECT_EQ(*It, StringRef("baz"));
   ASSERT_EQ(++It, End);
 }
+
+TEST(StringExtrasTest, splitCharForLoop) {
+  llvm::SmallVector<StringRef, 4> Result;
+  for (StringRef x : split("foo,bar,,baz", ','))
+    Result.push_back(x);
+  EXPECT_THAT(Result, testing::ElementsAre("foo", "bar", "", "baz"));
+}