-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Add missing overloads to flow rune proposal #120314
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.
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
Add missing overloads to flow rune proposal #120314
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Adds new String and Char overloads to support searching for char and Rune with StringComparison and startIndex/count parameters, plus a Char.Equals overload with StringComparison. Updates reference assembly, core implementations, span helpers, and expands tests accordingly.
- Adds IndexOf/LastIndexOf overloads for char and Rune with (startIndex, comparisonType) and (startIndex, count, comparisonType).
- Exposes Char.Equals(char, StringComparison) publicly and adds corresponding tests.
- Implements ordinal ignore-case search helpers and supporting SpanHelpers last-index routines.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
System.Runtime.Tests/System/StringTests.cs | Updated and expanded tests for new IndexOf/LastIndexOf overloads (char and Rune) with startIndex/count + comparison. |
System.Runtime.Tests/System/CharTests.cs | Added tests for new Char.Equals with StringComparison and adjusted existing equality tests. |
System.Runtime/ref/System.Runtime.cs | Added new public API surface for String and Char overloads. |
System.Private.CoreLib/src/System/String.Searching.cs | Implemented new overloads, publicized Rune variants, and added ordinal ignore-case logic for ranged search. |
System.Private.CoreLib/src/System/SpanHelpers.T.cs | Added LastIndexOfChar / LastIndexOfAnyChar helpers for performance support. |
System.Private.CoreLib/src/System/Char.cs | Made Char.Equals(char, StringComparison) public with XML docs. |
src/libraries/System.Private.CoreLib/src/System/String.Searching.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/String.Searching.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/CharTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs
Show resolved
Hide resolved
@Joy-less let us not stop continuing updating this PR so we can be ready when we decide to merge. Thanks! |
Understood, please could you help with this issue if it is clear? #120314 (comment) |
Let’s proceed with the following behavior:
I chose this behavior for the following reasons:
|
@tarekgh I have added the |
nit: this is correct, but I am wondering would be better to use Refers to: src/libraries/System.Private.CoreLib/src/System/String.Searching.cs:438 in 8abd5d3. [](commit_id = 8abd5d3, deletion_comment = False) |
@Joy-less I left minor comments. It looks good to me otherwise. Let's wait to resolve the F# issue and then we can proceed. |
I assume you mean |
I am kind of confused about the errors we're getting here:
Relevant test: [InlineData("ı", 'I', 0, int.MaxValue, StringComparison.CurrentCultureIgnoreCase, "tr-TR", 0)]
public static void IndexOf_SingleLetter_StringComparison(string s, char target, int startIndex, int count, StringComparison stringComparison, string? cultureName, int expected)
{
using (new ThreadCultureChange(cultureName))
{
if (count == int.MaxValue)
{
count = s.Length - startIndex;
}
if (startIndex == 0 && count == s.Length)
{
Assert.Equal(expected, s.IndexOf(target, stringComparison));
}
if (s.Length - startIndex == count)
{
Assert.Equal(expected, s.IndexOf(target, startIndex, stringComparison));
}
Assert.Equal(expected, s.IndexOf(target, startIndex, count, stringComparison));
ReadOnlySpan<char> targetSpan = [target];
int subIndex = s.AsSpan(startIndex, count).IndexOf(targetSpan, stringComparison);
Assert.Equal(expected, subIndex < 0 ? subIndex : startIndex + subIndex);
}
} On this line, the following method is called: Assert.Equal(expected, s.IndexOf(target, startIndex, count, stringComparison)); public int IndexOf(char value, int startIndex, int count, StringComparison comparisonType)
{
return comparisonType switch
{
StringComparison.CurrentCulture or StringComparison.CurrentCultureIgnoreCase => CultureInfo.CurrentCulture.CompareInfo.IndexOf(this, value, startIndex, count, GetCaseCompareOfComparisonCulture(comparisonType)),
StringComparison.InvariantCulture or StringComparison.InvariantCultureIgnoreCase => CompareInfo.Invariant.IndexOf(this, value, startIndex, count, GetCaseCompareOfComparisonCulture(comparisonType)),
StringComparison.Ordinal => IndexOf(value, startIndex, count),
StringComparison.OrdinalIgnoreCase => IndexOfCharOrdinalIgnoreCase(value, startIndex, count),
_ => throw new ArgumentException(SR.NotSupported_StringComparison, nameof(comparisonType)),
};
} The relevant method called is // in GetCaseCompareOfComparisonCulture method
// StringComparison.CurrentCultureIgnoreCase: 0x01 So if you run the method that the test calls in e.g. dotnetfiddle.net you get: Thread.CurrentThread.CurrentCulture = new CultureInfo("tr-TR");
Console.WriteLine(CultureInfo.CurrentCulture.CompareInfo.IndexOf("ı", 'I', 0, 1, (CompareOptions)0x01));
Not sure how it is returning |
@Joy-less please have a look at #106560. Please exclude this test case for Android for now. CC @matouskozak |
Okay, how do I exclude the test for Android? |
Put the Turkish |
@tarekgh All tests pass now, so this PR should be ready whenever the F# issue is resolved. |
@tarekgh Well, the pull request with the F# discussion now got merged with fixes for F#, does that mean this pull request is good to go? |
That PR merged just to unblock the code flow to that repo. We still need to discuss the issue and decide which way we should go with. |
I’m merging this now. If we need to make any changes related to F#, we can handle them together with the previously added APIs in the same classes. |
Closes #120015 proposal.
Adds the following APIs with refs and tests:
NOTE:This pull request ignores the two amendments listed in #120015 (comment). It should not be merged until those amendments are approved/rejected.
@tarekgh