[PATCH] D23981: Add StringRef::scan_between

[Zachary Turner via llvm-commits]

zturner added inline comments.

================
Comment at: include/llvm/ADT/StringRef.h:501
@@ +500,3 @@
+    /// Returns a StringRef representing the first occurrence of a string Result
+    /// such that \p Left + Result + \p Right is a substring of *this.  When
+    /// \p Left is empty, the result is taken from the beginning of the string
----------------
amccarth wrote:
> This description is not 100% accurate.  See my next comment.
The description could probably be improved.

================
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", "::"));
----------------
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, "")`.


https://reviews.llvm.org/D23981