Fix various LastIndexOf bugs when dealing with zero-length target substrings #34616
Conversation
- string.LastIndexOf(string.Empty) shouldn't perform -1 adjustment - MemoryExtensions.LastIndexOf(ROS<char>, string.Empty) shouldn't return 0 - Tighten up some existing unit tests
GrabYourPitchforks
added
area-System.Runtime
breaking-change
GrabYourPitchforks
requested review from
adamsitnik and
bartonjs
and removed request for
ahsonkhan
|
Note to @tarekgh: This PR doesn't have the "avoid validating |
|
CC @safern just in case he is touching some of the changed files here. |
tarekgh
left a comment
tarekgh
left a comment
There was a problem hiding this comment.
I like the change and now our behavior is more understandable. only one thing, we need to document this as a breaking change. right? if so, can we ensure this is written so we can point users to the doc when anyone complains? Thanks for clearing this area.
|
This issue is marked with a breaking change tag. In theory it should be caught in any pre-release sweep we do to account for such changes in the docs. |
| Assert.Equal(10, span.LastIndexOf(value.AsSpan(), StringComparison.CurrentCulture)); | ||
| Assert.Equal(10, span.LastIndexOf(value.AsSpan(), StringComparison.CurrentCultureIgnoreCase)); | ||
| } | ||
| } |
There was a problem hiding this comment.
not related to your changes: IMHO this test (IndexOf_AllSubstrings) is very complex and is trying to test too many test cases at the same time. It would be better to split it into few smaller tests with self-describing names
There was a problem hiding this comment.
Agreed, it's a bit of a bear. Do you have recommendations for how it might naturally be split?
There was a problem hiding this comment.
I think that the two if blocks could become separate tests: value.Length == 0, s.Length == 0
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes #13382.
Fixes #13383.
string.LastIndexOfandMemoryExtensions.LastIndexOfare inconsistent with how they handle zero-length target strings with variousStringComparisonarguments. In some cases they returnstring.Length - 1. In some cases they returnspan.Length. And in some cases they return0.This PR normalizes the behavior of all of these APIs so that a zero-length target substring is always found at the end of the search space. Since
LastIndexOf's parameters vary significantly fromIndexOf's parameters, I also included a code comment right in front ofstring.IndexOf(string)that explains the concept of "search space" vis-à-vis this particular API.There's also some small comment cleanup throughout the code paths which deal with these checks.
This PR was extracted from #1514 into its own standalone review. This PR addresses only
LastIndexOfand doesn't address the zero-weight code point issue tracked by that PR.