Skip to content

Add missing early-exit for LastIndexOfOrdinal #33601

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1 commit merged into from
Mar 15, 2020
Merged

Add missing early-exit for LastIndexOfOrdinal #33601

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,11 +1362,29 @@ public int LastIndexOf(string source, string value, int startIndex, int count, C

internal static int LastIndexOfOrdinal(string source, string value, int startIndex, int count, bool ignoreCase)
{
Debug.Assert(source != null);
Debug.Assert(value != null);

if (GlobalizationMode.Invariant)
{
return InvariantLastIndexOf(source, value, startIndex, count, ignoreCase);
}

// For ordinal (non-linguistic) comparisons, an empty target string is always
// found at the end of the search space, and a non-empty target string
// can never be found within an empty search space. This assumption is not
// valid for linguistic comparisons, including InvariantCulture comparisons.

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

@GrabYourPitchforks GrabYourPitchforks Mar 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

if (count == 0)
{
return -1;
}

return LastIndexOfOrdinalCore(source, value, startIndex, count, ignoreCase);
}

Expand Down