[ADT] Add `StringRef::rsplit(StringRef Separator)`.

[MTC]

Summary

Add StringRef::rsplit(StringRef Separator) to achieve the function of getting the tail substring according to the separator. A typical usage is to get data in std::basic_string::data.

[MTC]

ping.

[xbolva00]

lg

[vsk]

Hi @MTC, thanks for the patch.

With this patch, the logic for split would be duplicated 3 times now in StringRef.h. I think it'd be nice to reuse some code by moving the core logic into a private method of StringRef, say, splitAtIndex(size_t). Then, each (r)split(char | StringRef) can use the common helper.

Relatedly, an rsplit(char) overload seems to be missing here.

[zturner]

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

[MTC]

This update consists of two parts

• Make rsplit(char) reuse rsplit(StringRef).
• Make split(char) reuse split(StringRef).

Do I need to divide the patch into two parts? One is to add rsplit(StringRef), the other is to refactor rsplit(char) and split(char).

[MTC]

In D47406#1120355, @vsk wrote:

Hi @MTC, thanks for the patch.

With this patch, the logic for split would be duplicated 3 times now in StringRef.h. I think it'd be nice to reuse some code by moving the core logic into a private method of StringRef, say, splitAtIndex(size_t). Then, each (r)split(char | StringRef) can use the common helper.

Relatedly, an rsplit(char) overload seems to be missing here.

First of all, sorry for the long delay!

Thanks for your review, @vsk! I do agree with you that there is need some work to remove the duplicates code. However I think zturner's approach is more neat, what do you think?

[MTC]

In D47406#1120360, @zturner wrote:

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

Thanks for your advice, @zturner! I have refactored split(char) and rsplit(char) to use StringRef version. I think the code is more clean now.

[zturner]

include/llvm/ADT/StringRef.h

755 ↗

(On Diff #150335)

I can't decide if it would make more sense to have the "not found" case return a pair (LHS, RHS) where (LHS == "") && (*this == RHS)? What do you think?

[vsk]

In D47406#1125104, @MTC wrote:

In D47406#1120360, @zturner wrote:

rsplit(char) already exists. That said, another way to reuse code would be
to have rsplit(char) call the StringRef version

Thanks for your advice, @zturner! I have refactored split(char) and rsplit(char) to use StringRef version. I think the code is more clean now.

LGTM. Please wait to see if Zachary has any further feedback. Thanks!

include/llvm/ADT/StringRef.h

755 ↗

(On Diff #150335)

I think this makes sense, but would make more sense as a follow-up commit. I don't think it needs to be a prerequisite for adding an rsplit overload.

[MTC]

include/llvm/ADT/StringRef.h

755 ↗

(On Diff #150335)

Yeah, I also think this is more reasonable.

1. split() find the first occurrence, perform a lookup from start to end. Given "hello" and separator 'w', the return will be "hello" + "".
2. rsplit() find the last occurrence, perfrom a lookup from end to start. Given "hello" and separator'w', the return should be "" + "hello".

If you all think this is more reasonable, I will give another patch to implement this. After all, current patch is for adding rsplit(StringRef). What do you think?

[zturner]

lgtm

llvm/trunk/include/llvm/ADT/StringRef.h

public:
     /// such that (*this == LHS + Separator + RHS) is true and RHS is
     /// maximal. If \p Separator is not in the string, then the result is a
     /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").
     ///
     /// \param Separator The character to split on.
     /// \returns The split substrings.
     LLVM_NODISCARD
     std::pair<StringRef, StringRef> split(char Separator) const {
-      size_t Idx = find(Separator);
-      if (Idx == npos)
-        return std::make_pair(*this, StringRef());
-      return std::make_pair(slice(0, Idx), slice(Idx+1, npos));
+      return split(StringRef(&Separator, 1));
     }
 
     /// Split into two substrings around the first occurrence of a separator
     /// string.
     ///
     /// If \p Separator is in the string, then the result is a pair (LHS, RHS)
     /// such that (*this == LHS + Separator + RHS) is true and RHS is
     /// maximal. If \p Separator is not in the string, then the result is a
     /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").
     ///
     /// \param Separator - The string to split on.
     /// \return - The split substrings.
     LLVM_NODISCARD
     std::pair<StringRef, StringRef> split(StringRef Separator) const {
       size_t Idx = find(Separator);
       if (Idx == npos)
         return std::make_pair(*this, StringRef());
       return std::make_pair(slice(0, Idx), slice(Idx + Separator.size(), npos));
     }
 
+    /// Split into two substrings around the last occurrence of a separator
+    /// string.
+    ///
+    /// If \p Separator is in the string, then the result is a pair (LHS, RHS)
+    /// such that (*this == LHS + Separator + RHS) is true and RHS is
+    /// minimal. If \p Separator is not in the string, then the result is a
+    /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").
+    ///
+    /// \param Separator - The string to split on.
+    /// \return - The split substrings.
+    LLVM_NODISCARD
+    std::pair<StringRef, StringRef> rsplit(StringRef Separator) const {
+      size_t Idx = rfind(Separator);
+      if (Idx == npos)
+        return std::make_pair(*this, StringRef());
+      return std::make_pair(slice(0, Idx), slice(Idx + Separator.size(), npos));
+    }
+
     /// Split into substrings around the occurrences of a separator string.
     ///
     /// Each substring is stored in \p A. If \p MaxSplit is >= 0, at most
     /// \p MaxSplit splits are done and consequently <= \p MaxSplit + 1
     /// elements are added to A.
     /// If \p KeepEmpty is false, empty strings are not added to \p A. They
     /// still count when considering \p MaxSplit
     /// An useful invariant is that
public:
     /// such that (*this == LHS + Separator + RHS) is true and RHS is
     /// minimal. If \p Separator is not in the string, then the result is a
     /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").
     ///
     /// \param Separator - The character to split on.
     /// \return - The split substrings.
     LLVM_NODISCARD
     std::pair<StringRef, StringRef> rsplit(char Separator) const {
-      size_t Idx = rfind(Separator);
-      if (Idx == npos)
-        return std::make_pair(*this, StringRef());
-      return std::make_pair(slice(0, Idx), slice(Idx+1, npos));
+      return rsplit(StringRef(&Separator, 1));
     }
 
     /// Return string with consecutive \p Char characters starting from the
     /// the left removed.
     LLVM_NODISCARD
     StringRef ltrim(char Char) const {
       return drop_front(std::min(Length, find_first_not_of(Char)));
     }

llvm/trunk/unittests/ADT/StringRefTest.cpp

TEST(StringRefTest, Split) {
   EXPECT_EQ(std::make_pair(StringRef("h"), StringRef("llo")),
             Str.rsplit('e'));
   EXPECT_EQ(std::make_pair(StringRef(""), StringRef("ello")),
             Str.rsplit('h'));
   EXPECT_EQ(std::make_pair(StringRef("hel"), StringRef("o")),
             Str.rsplit('l'));
   EXPECT_EQ(std::make_pair(StringRef("hell"), StringRef("")),
             Str.rsplit('o'));
+
+  EXPECT_EQ(std::make_pair(StringRef("he"), StringRef("o")),
+        Str.rsplit("ll"));
+  EXPECT_EQ(std::make_pair(StringRef(""), StringRef("ello")),
+        Str.rsplit("h"));
+  EXPECT_EQ(std::make_pair(StringRef("hell"), StringRef("")),
+        Str.rsplit("o"));
+  EXPECT_EQ(std::make_pair(StringRef("hello"), StringRef("")),
+        Str.rsplit("::"));
+  EXPECT_EQ(std::make_pair(StringRef("hel"), StringRef("o")),
+        Str.rsplit("l"));
 }
 
 TEST(StringRefTest, Split2) {
   SmallVector<StringRef, 5> parts;
   SmallVector<StringRef, 5> expected;
 
   expected.push_back("ab"); expected.push_back("c");
   StringRef(",ab,,c,").split(parts, ",", -1, false);