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

[Vedant Kumar via Phabricator via llvm-commits]

vsk accepted this revision.
vsk added a comment.
This revision is now accepted and ready to land.

In https://reviews.llvm.org/D47406#1125104, @MTC wrote:

> In https://reviews.llvm.org/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!



================
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 == "").
+    ///
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D47406