[PATCH] D23981: Add StringRef::scan_between

[Adrian McCarthy via llvm-commits]

amccarth added inline comments.

================
Comment at: unittests/ADT/StringRefTest.cpp:382
@@ +381,3 @@
+  EXPECT_EQ("ScanBetween", Str.scan_between("::", "()"));
+  EXPECT_EQ("::ScanBetween()", Str.scan_between("StringRefTest", "ABCDE"));
+  EXPECT_EQ("", Str.scan_between("ABCDE", "::"));
----------------
zturner wrote:
> amccarth wrote:
> > The expectation here surprised me.  Given that "ABCDE" doesn't exist in the string, I'd expect there to be no match (as with the next example).  But there's an asymmetry here.
> > 
> > I'm not necessarily opposed to this, but I think it's worth discussing since surprising behavior from an API leads to misuse of the API.  At a minimum, the description of the method needs to be corrected to make this behavior explicit.
> > 
> > Consider `Foo.scan_between("(", ")")`.  Would you really want a non-empty result if `Foo` did not have a matched pair of parentheses?
> When I came up with these semantics, I decided to treat "I didn't find the string" not as an error, but as an "I made it to the end, keep going".
> 
> This roughly matches the semantics of `split()`, where if if `A.split(B)` doesn't find `B`, it returns `(A, "")`.
`split` doesn't document what it does if you pass an empty string as a separator.  I'd expect an empty string to match at the first position, which is what happens with `std::string::find`.  If `scan_between` were to use the same semantics as `split`, I'd expect `A.scan_between("", "")` to return an empty string rather than `A`.


https://reviews.llvm.org/D23981