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

mgorny · mgorny · commit 66e06cc8cba3 · 2021-10-22T12:27:46.000+02:00
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. Differential Revision: https://reviews.llvm.org/D110535
diff --git a/lldb/source/Host/common/File.cpp b/lldb/source/Host/common/File.cpp
@@ -772,7 +772,7 @@ mode_t File::ConvertOpenOptionsForPOSIXOpen(OpenOptions open_options) {
 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))
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationClient.cpp
@@ -355,7 +355,7 @@ void GDBRemoteCommunicationClient::GetRemoteQSupported() {
     // 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+")
@@ -1609,7 +1609,7 @@ Status GDBRemoteCommunicationClient::GetMemoryRegionInfo(
           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))
diff --git a/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp b/lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
@@ -3637,7 +3637,7 @@ GDBRemoteCommunicationServerLLGS::Handle_qSaveCore(
   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
diff --git a/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp b/lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp
@@ -375,7 +375,7 @@ 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);
@@ -1405,7 +1405,7 @@ size_t ProcessGDBRemote::UpdateThreadIDsFromStopReplyThreadsValue(
 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);
@@ -4973,7 +4973,7 @@ llvm::Expected<bool> ProcessGDBRemote::SaveCore(llvm::StringRef outfile) {
     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);
     }
diff --git a/llvm/include/llvm/ADT/StringExtras.h b/llvm/include/llvm/ADT/StringExtras.h
@@ -505,6 +505,7 @@ class ListSeparator {
 class SplittingIterator
     : public iterator_facade_base<SplittingIterator, std::forward_iterator_tag,
                                   StringRef> {
+  char SeparatorStorage;
   StringRef Current;
   StringRef Next;
   StringRef Separator;
@@ -515,18 +516,43 @@ class SplittingIterator
     ++*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;
   }
 };
@@ -536,26 +562,21 @@ class SplittingIterator
 /// 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
 
diff --git a/llvm/lib/IR/DataLayout.cpp b/llvm/lib/IR/DataLayout.cpp
@@ -260,12 +260,12 @@ Error DataLayout::parseSpecifier(StringRef 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.
@@ -274,7 +274,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
 
     if (Tok == "ni") {
       do {
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
         Rest = Split.second;
         unsigned AS;
@@ -315,7 +315,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
       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))
@@ -327,7 +327,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
       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))
@@ -342,7 +342,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
       // 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;
@@ -352,7 +352,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
 
         // 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;
@@ -393,7 +393,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
       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))
@@ -410,7 +410,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
       // 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;
@@ -439,7 +439,7 @@ Error DataLayout::parseSpecifier(StringRef Desc) {
         LegalIntWidths.push_back(Width);
         if (Rest.empty())
           break;
-        if (Error Err = split(Rest, ':', Split))
+        if (Error Err = ::split(Rest, ':', Split))
           return Err;
       }
       break;
diff --git a/llvm/unittests/ADT/StringExtrasTest.cpp b/llvm/unittests/ADT/StringExtrasTest.cpp
@@ -8,6 +8,7 @@
 
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/Support/raw_ostream.h"
+#include "gmock/gmock.h"
 #include "gtest/gtest.h"
 
 using namespace llvm;
@@ -276,7 +277,7 @@ TEST(StringExtrasTest, toStringAPSInt) {
 }
 
 TEST(StringExtrasTest, splitStringRef) {
-  auto Spl = Split("foo<=>bar<=><=>baz", "<=>");
+  auto Spl = split("foo<=>bar<=><=>baz", "<=>");
   auto It = Spl.begin();
   auto End = Spl.end();
 
@@ -291,8 +292,15 @@ TEST(StringExtrasTest, splitStringRef) {
   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();
 
@@ -306,3 +314,10 @@ TEST(StringExtrasTest, splItChar) {
   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"));
+}
