[PATCH] D23981: Add StringRef::scan_between
Zachary Turner via llvm-commits
llvm-commits at lists.llvm.org
Mon Aug 29 13:26:18 PDT 2016
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
> 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", "::"));
> 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, "")`.
More information about the llvm-commits