[PATCH] D23981: Add StringRef::scan_between

[Adrian McCarthy via llvm-commits]

amccarth 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
----------------
This description is not 100% accurate.  See my next comment.

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


https://reviews.llvm.org/D23981