[PATCH] D23981: Add StringRef::scan_between

Adrian McCarthy via llvm-commits llvm-commits at lists.llvm.org
Mon Aug 29 13:18:45 PDT 2016

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?


More information about the llvm-commits mailing list