Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with StringComparison #27319

ahsonkhan · 2018-02-21T07:28:39Z

Part of https://github.com/dotnet/corefx/issues/21395#issuecomment-359906138

Equals
CompareTo
IndexOf ~~(fast span only)~~
Contains ~~(fast span only)~~

Depends on: dotnet/coreclr#16467

TODO:

Add more tests
~~Verify correctness on Unix~~
Can we expose string.IndexOf with out int matchedLength parameter? Can we expose string.Contains with StringComparison comparisonType parameter? If not, these APIs cannot be implemented for portable span.

cc @jkotas, @stephentoub, @KrzysztofCwalina, @tarekgh, @JeremyKuhne, @joshfree

…ringComparison

…ringAPIs

tarekgh · 2018-02-22T18:19:34Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+                return span.SequenceEqual<char>(value);
+            }
+
+            return span.ToString().Equals(value.ToString(), comparisonType);


return span.ToString().Equals(value.ToString(), comparisonType); [](start = 12, length = 64)

We don't have any optimization for ordinal ignore case when having ascii cases as we did in coreclr? this apply for all introduced APIs here

Will address outside the PR (see https://github.com/dotnet/corefx/issues/27379).

tarekgh · 2018-02-22T18:20:38Z

src/System.Memory/src/System/MemoryExtensions.Portable.cs

+        /// <param name="value">The value to compare with the source span.</param>
+        /// <param name="comparisonType">One of the enumeration values that determines how the <paramref name="span"/> and <paramref name="value"/> are compared.</param>
+        /// </summary>
+        public static int CompareTo(this ReadOnlySpan<char> span, ReadOnlySpan<char> value, StringComparison comparisonType) 


why not optimizing for the ordinal cases?

Since we don't already have an optimized ordinal CompareTo (unlike the other APIs), I will address this outside the PR.

Filed https://github.com/dotnet/corefx/issues/27379

Thanks for tracking this issue.

tarekgh · 2018-02-22T18:24:20Z

src/System.Memory/tests/ReadOnlySpan/CompareTo.cs

+
+using System.Globalization;
+using System.Threading;
+using Xunit;


in general for tests, I prefer to edit the tests in https://github.com/dotnet/corefx/blob/master/src/System.Globalization/tests/CompareInfo and inject the Span cases inside the test cases. by doing that we'll ensure the string and the span behavior is identical and in the future any added more test data will get automatically applied to both the strings and the spans

There is some overlap here: https://github.com/dotnet/corefx/blob/master/src/System.Runtime/tests/System/StringTests.cs#L589

Also, the CompareInfo tests are testing CompareOptions, which Span APIs do not have.

Would be better then add the span tests to the string tests you have pointed at. I just want to ensure the string and span behaving exactly same.

That is where I borrowed the tests from. I had to make some minor adjustments which is why it doesn't seem worth the effort to move the tests back.

Also, span has specific tests that are unique to it, so this way all the span tests live in one place (and one assembly).

I didn't mean to borrow the test but inject the span tests into the string tests. it'll pay off when adding more test cases or data to these tests. Maintaining 2 places which we need to guarantee the consistency would be hard.

For Unique Span tests, that fine to split these.

If the string APIs start calling the span APIs instead, we would get the coverage without having to update the tests.

I can update the tests if it turns out we can't call the span APIs from the string APIs due to perf or other reason.

See https://github.com/dotnet/corefx/issues/28238

ahsonkhan · 2018-02-27T21:41:34Z

@dotnet-bot test this please

ahsonkhan · 2018-02-28T01:05:17Z

@dotnet-bot test Tizen armel Debug Build

…ringAPIs

ahsonkhan · 2018-02-28T05:02:15Z

Windows x86 Release Build
Windows.10.Amd64.Open-Release-x86
https://mc.dot.net/#/user/ahsonkhan/pr~2Fjenkins~2Fdotnet~2Fcorefx~2Fmaster~2F/test~2Ffunctional~2Fcli~2F/3941a9303e4dd657a0db10fbfaf4d751a67ed57d/workItem/System.Net.Http.Functional.Tests
System.Net.Http.Functional.Tests.HttpCookieProtocolTests/GetAsyncWithRedirect_SetCookieContainer_CorrectCookiesSent

System.Net.Http.HttpRequestException : An error occurred while sending the request.
---- System.Net.Http.WinHttpException : The server returned an invalid or unrecognized response

   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Net.Http\src\System\Net\Http\HttpClient.cs:line 469
   at System.Net.Http.Functional.Tests.HttpCookieProtocolTests.<GetAsyncWithRedirect_SetCookieContainer_CorrectCookiesSent>b__15_0(Uri url) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Net.Http\tests\FunctionalTests\HttpCookieProtocolTests.cs:line 319
   at System.Net.Test.Common.LoopbackServer.<>c__DisplayClass11_0.<<CreateClientAndServerAsync>b__0>d.MoveNext() in D:\j\workspace\windows-TGrou---f8ac6754\src\Common\tests\System\Net\Http\LoopbackServer.cs:line 84
--- End of stack trace from previous location where exception was thrown ---
   at System.Net.Test.Common.LoopbackServer.CreateServerAsync(Func`2 funcAsync, Options options) in D:\j\workspace\windows-TGrou---f8ac6754\src\Common\tests\System\Net\Http\LoopbackServer.cs:line 67
   at System.Net.Http.Functional.Tests.HttpCookieProtocolTests.GetAsyncWithRedirect_SetCookieContainer_CorrectCookiesSent() in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Net.Http\tests\FunctionalTests\HttpCookieProtocolTests.cs:line 305
--- End of stack trace from previous location where exception was thrown ---
----- Inner Stack Trace -----
   at System.Threading.Tasks.RendezvousAwaitable`1.GetResult() in D:\j\workspace\windows-TGrou---f8ac6754\src\Common\src\System\Threading\Tasks\RendezvousAwaitable.cs:line 62
   at System.Net.Http.WinHttpHandler.StartRequest(WinHttpRequestState state) in D:\j\workspace\windows-TGrou---f8ac6754\src\System.Net.Http.WinHttpHandler\src\System\Net\Http\WinHttpHandler.cs:line 863

https://github.com/dotnet/corefx/issues/27390

…ringComparison (dotnet/corefx#27319) * Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with StringComparison * Respond to recent change AsReadOnlySpan -> AsSpan * Remove the out parameter from IndexOf * Add tests and avoid allocations for StringComparison.Ordinal * Remove duplicate definition of SoftHyphen Commit migrated from dotnet/corefx@fdabc62

Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with St…

b10228f

…ringComparison

ahsonkhan added the area-System.Memory label Feb 21, 2018

ahsonkhan self-assigned this Feb 21, 2018

ahsonkhan mentioned this pull request Feb 21, 2018

Add ReadOnlySpan string-like Equals/CompareTo/IndexOf/Contains API with globalization support dotnet/coreclr#16467

Merged

ahsonkhan added the blocked Issue/PR is blocked on something - see comments label Feb 21, 2018

ahsonkhan added 6 commits February 21, 2018 14:22

Merge branch 'master' of https://github.com/dotnet/corefx into SpanSt…

67bf3a9

…ringAPIs

Respond to recent change AsReadOnlySpan -> AsSpan

bec2f09

Remove the out parameter from IndexOf

8dd0adf

Add tests and avoid allocations for StringComparison.Ordinal

23589ab

Merge branch 'master' of https://github.com/dotnet/corefx into SpanSt…

9b1ce10

…ringAPIs

Remove duplicate definition of SoftHyphen

06c34b4

joshfree requested a review from tarekgh February 22, 2018 18:12

tarekgh reviewed Feb 22, 2018

View reviewed changes

joshfree removed the blocked Issue/PR is blocked on something - see comments label Feb 27, 2018

Merge branch 'master' of https://github.com/dotnet/corefx into SpanSt…

3941a93

…ringAPIs

Merge branch 'master' into SpanStringAPIs

68feaf7

ahsonkhan merged commit fdabc62 into dotnet:master Feb 28, 2018

ahsonkhan deleted the SpanStringAPIs branch February 28, 2018 19:09

karelz added this to the 2.1.0 milestone Mar 10, 2018

This was referenced Jan 31, 2020

Optimize string-like portable Span APIs for Ordinal and OrdinalIgnoreCase dotnet/runtime#25156

Closed

Consolidate the string and span tests for the string-like APIs dotnet/runtime#25527

Closed

Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with StringComparison #27319

Add ROSpan Equals/CompareTo/IndexOf/Contains string-like APIs with StringComparison #27319

Uh oh!

Conversation

ahsonkhan commented Feb 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahsonkhan commented Feb 27, 2018

Uh oh!

ahsonkhan commented Feb 28, 2018

Uh oh!

ahsonkhan commented Feb 28, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ahsonkhan commented Feb 21, 2018 •

edited

Loading