Add missing early-exit for LastIndexOfOrdinal #33601

GrabYourPitchforks · 2020-03-14T23:29:15Z

Fix #33596. We added the appropriate early-exit for IndexOfOrdinal but missed it for LastIndexOfOrdinal.

GrabYourPitchforks · 2020-03-14T23:33:49Z

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs

+
+            if (value.Length == 0)
+            {
+                return Math.Max(0, startIndex - count + 1);


This line is because the behavior of LastIndexOf is nutty. See the comment below (lifted from PR #1514) for more info.

runtime/src/libraries/System.Private.CoreLib/src/System/String.Searching.cs

Lines 277 to 298 in 52aea3b

* For LastIndexOf specifially, overloads which take a 'startIndex' and 'count' behave differently

* than their IndexOf counterparts. 'startIndex' is the index of the last char element that should

* be considered when performing the search. For example, if startIndex = 4, then the caller is

* indicating "when finding the match I want you to include the char element at index 4, but not

* any char elements past that point."

*

* idx = 0123456 ("abcdefg".Length = 7)

* So, if the search string is "abcdefg", startIndex = 5 and count = 3, then the search space will

* ~~~ be the substring "def", as highlighted to the left.

* Essentially: "the search space should be of length 3 chars and should end *just after* the char

* element at index 5."

*

* Since this behavior can introduce off-by-one errors in the boundary cases, we allow startIndex = -1

* with a zero-length 'searchString' (treated as equivalent to startIndex = 0), and we allow

* startIndex = searchString.Length (treated as equivalent to startIndex = searchString.Length - 1).

*

* Note also that this behavior can introduce errors when dealing with UTF-16 surrogate pairs.

* If the search string is the 3 chars "[BMP][HI][LO]", startIndex = 1 and count = 2, then the

* ~~~~~~~~~ search space wil be the substring "[BMP][ HI]".

* This means that the char [HI] is incorrectly seen as a standalone high surrogate, which could

* lead to incorrect matching behavior, or it could cause LastIndexOf to incorrectly report that

* a zero-weight character could appear between the [HI] and [LO] chars.

GrabYourPitchforks · 2020-03-14T23:35:50Z

@dotnet-bot help

GrabYourPitchforks · 2020-03-14T23:36:56Z

@dotnet-bot test runtime (Libraries Test Run checked coreclr Windows_NT x64 Debug)
@dotnet-bot test runtime (Libraries Test Run checked coreclr Windows_NT x86 Release)

GrabYourPitchforks · 2020-03-14T23:37:42Z

/azp list

azure-pipelines · 2020-03-14T23:37:47Z

CI/CD Pipelines for this repository: coreclr-gc-longrunning coreclr-gc-simulator runtime-coreclr outerloop runtime-coreclr crossgen2 runtime-libraries outerloop runtime-libraries outerloop-windows runtime-libraries outerloop-linux runtime-libraries outerloop-osx runtime runtime-libraries enterprise-linux runtime-libraries stress-http runtime-libraries stress-ssl runtime-live-build runtime-coreclr crossgen2 outerloop coreclr-release-outerloop-nightly sync-runtime-to-mono dotnet-runtime-perf runtime-coreclr crossgen2-composite

GrabYourPitchforks · 2020-03-14T23:38:50Z

/azp run runtime (Libraries Test Run checked coreclr Windows_NT x86 Release)
/azp run runtime (Libraries Test Run checked coreclr Windows_NT x64 Debug)

azure-pipelines · 2020-03-14T23:38:55Z

No pipelines are associated with this pull request.

GrabYourPitchforks · 2020-03-14T23:39:42Z

Don't worry, bots. I still love you. ❤️

GrabYourPitchforks · 2020-03-14T23:40:29Z

/azp run runtime

azure-pipelines · 2020-03-14T23:40:48Z

Azure Pipelines successfully started running 1 pipeline(s).

ghost · 2020-03-15T00:55:23Z

Hello @GrabYourPitchforks!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

Add missing early-exit for LastIndexOfOrdinal

210e525

GrabYourPitchforks added the area-System.Globalization label Mar 14, 2020

GrabYourPitchforks added this to the 5.0 milestone Mar 14, 2020

GrabYourPitchforks requested a review from tarekgh March 14, 2020 23:29

tarekgh approved these changes Mar 14, 2020

View reviewed changes

GrabYourPitchforks commented Mar 14, 2020

View reviewed changes

GrabYourPitchforks added the auto-merge label Mar 15, 2020

ghost merged commit a46a792 into dotnet:master Mar 15, 2020

GrabYourPitchforks deleted the upd_lio branch March 15, 2020 03:53

ghost locked as resolved and limited conversation to collaborators Dec 10, 2020

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add missing early-exit for LastIndexOfOrdinal #33601

Add missing early-exit for LastIndexOfOrdinal #33601

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

ghost commented Mar 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	* For LastIndexOf specifially, overloads which take a 'startIndex' and 'count' behave differently
	* than their IndexOf counterparts. 'startIndex' is the index of the last char element that should
	* be considered when performing the search. For example, if startIndex = 4, then the caller is
	* indicating "when finding the match I want you to include the char element at index 4, but not
	* any char elements past that point."
	*
	* idx = 0123456 ("abcdefg".Length = 7)
	* So, if the search string is "abcdefg", startIndex = 5 and count = 3, then the search space will
	* ~~~ be the substring "def", as highlighted to the left.
	* Essentially: "the search space should be of length 3 chars and should end just after the char
	* element at index 5."
	*
	* Since this behavior can introduce off-by-one errors in the boundary cases, we allow startIndex = -1
	* with a zero-length 'searchString' (treated as equivalent to startIndex = 0), and we allow
	* startIndex = searchString.Length (treated as equivalent to startIndex = searchString.Length - 1).
	*
	* Note also that this behavior can introduce errors when dealing with UTF-16 surrogate pairs.
	* If the search string is the 3 chars "[BMP][HI][LO]", startIndex = 1 and count = 2, then the
	* ~~~~~~~~~ search space wil be the substring "[BMP][ HI]".
	* This means that the char [HI] is incorrectly seen as a standalone high surrogate, which could
	* lead to incorrect matching behavior, or it could cause LastIndexOf to incorrectly report that
	* a zero-weight character could appear between the [HI] and [LO] chars.

Add missing early-exit for LastIndexOfOrdinal #33601

Add missing early-exit for LastIndexOfOrdinal #33601

Uh oh!

Conversation

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

GrabYourPitchforks commented Mar 14, 2020

Uh oh!

azure-pipelines bot commented Mar 14, 2020

Uh oh!

ghost commented Mar 15, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.