[ADT] Change the behavior of `StringRef::rsplit()` when the separator is not found.

[MTC]

Summary

For StringRef::split(), it performs the search from the front to the back to find the first occurence of the separator, so when the separator is not found, returning <*this, ""> makes sense. For SrtingRef::rsplit(), it performs the search from the back to the front to find the "first" occurence of the separator too, so when the separator is not found, returning <"", *this> makes more sense.

[vsk]

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

[xbolva00]

Makes sense, thanks

[MTC]

In D47973#1127309, @vsk wrote:

Thanks for the patch. The StringRef change itself looks fine, but please make sure to update the callsites in-tree (e.g in llvm, clang, lldb etc).

Thanks for your reminder, vsk! Sorry for my negligence, I thought these corner cases were definitely included in the test case, so after the test was successful, I thought there was no potential problem.

After I re-examined some of the usages of StringRef::rsplit(), I think I need to abandon this patch.

There are three reasons for this:

• With some usages, the string may need to be rsplit() through multiple separator. After splitting using the first separator, we can always use sample_string.rsplit(separator).first for the next split stage. However for this patch, we have to use sample_sring.rsplit(separator).first if the separator is in sample_string, and use sample_string.rsplit(separator).second if the separator is not in sample_string. The code is not clean!
• Changing the behavior of existing libraries is dangerous especially it is difficult for me to figure out all the code logic of the StringRef::rsplit() usages.
• Like the rsplit() in other programming languages, e.g python. rsplit() in python splits string from the right at the specified separator and returns a list of strings, and rsplit() guarantee list[0] always have values even if the separator isn't in the string. So for StringRef::rsplit(), even though StringRef::rsplit() returns a pair instead of list, we should guarantee that pair.first always has value for subsequent use.

Sorry for the lack of thinking about this patch!

What do you think, @xbolva00, @vsk!

include/llvm/ADT/StringRef.h

std::pair<StringRef, StringRef> split(StringRef Separator) const {
         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 == "").
+    /// 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 (LHS == "") and (*this == 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(StringRef(), *this);
       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.
public:
     /// \param KeepEmpty - True if empty substring should be added.
     void split(SmallVectorImpl<StringRef> &A, char Separator, int MaxSplit = -1,
                bool KeepEmpty = true) const;
 
     /// Split into two substrings around the last occurrence of a separator
     /// character.
     ///
     /// 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 == "").
+    /// 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 (LHS == "") and (*this == RHS).
     ///
     /// \param Separator - The character to split on.
     /// \return - The split substrings.
     LLVM_NODISCARD
     std::pair<StringRef, StringRef> rsplit(char Separator) const {
       return rsplit(StringRef(&Separator, 1));
     }
 

unittests/ADT/StringRefTest.cpp

EXPECT_EQ(std::make_pair(StringRef("h"), StringRef("llo")),
             Str.split('e'));
   EXPECT_EQ(std::make_pair(StringRef(""), StringRef("ello")),
             Str.split('h'));
   EXPECT_EQ(std::make_pair(StringRef("he"), StringRef("lo")),
             Str.split('l'));
   EXPECT_EQ(std::make_pair(StringRef("hell"), StringRef("")),
             Str.split('o'));
 
-  EXPECT_EQ(std::make_pair(StringRef("hello"), StringRef("")),
+  EXPECT_EQ(std::make_pair(StringRef(""), StringRef("hello")),
             Str.rsplit('X'));
   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("")),
+  EXPECT_EQ(std::make_pair(StringRef(""), StringRef("hello")),
         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;