<div dir="ltr">lgtm</div><br><div class="gmail_quote"><div dir="ltr">On Thu, Jun 7, 2018 at 7:35 PM Henry Wong via Phabricator <<a href="mailto:reviews@reviews.llvm.org">reviews@reviews.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">MTC added inline comments.<br>
<br>
<br>
================<br>
Comment at: include/llvm/ADT/StringRef.h:755<br>
+    /// minimal. If \p Separator is not in the string, then the result is a<br>
+    /// pair (LHS, RHS) where (*this == LHS) and (RHS == "").<br>
+    ///<br>
----------------<br>
vsk wrote:<br>
> zturner wrote:<br>
> > 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?<br>
> 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.<br>
Yeah, I also think this is more reasonable.<br>
<br>
  # `split()` find the first occurrence, perform a lookup from start to end. Given "hello" and separator 'w', the return will be `"hello"` + `""`. <br>
<br>
  # `rsplit()` find the last occurrence, perfrom a lookup from end to start. Given "hello" and separator'w', the return should be `""` + `"hello"`.<br>
<br>
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?<br>
<br>
<br>
<br>
<br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D47406" rel="noreferrer" target="_blank">https://reviews.llvm.org/D47406</a><br>
<br>
<br>
<br>
</blockquote></div>