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

[Henry Wong via Phabricator via llvm-commits]

MTC added inline comments.


================
Comment at: include/llvm/ADT/StringRef.h:755
+    /// minimal. If \p Separator is not in the string, then the result is a
+    /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").
+    ///
----------------
vsk wrote:
> zturner wrote:
> > 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?
> 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.
Yeah, I also think this is more reasonable.

  # `split()` find the first occurrence, perform a lookup from start to end. Given "hello" and separator 'w', the return will be `"hello"` + `""`. 

  # `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?





Repository:
  rL LLVM

https://reviews.llvm.org/D47406