Add specific tests for {RO}Span.IndexOfAny<char> by benaadams · Pull Request #32069 · dotnet/corefx

benaadams · 2018-09-01T23:26:11Z

Supporting dotnet/coreclr#19790

krwq · 2018-09-02T06:16:50Z

+        }
+
+        [Fact]
+        public static void DefaultFilledIndexOfTwo_Char()


I'm not sure what is this test testing.

Not entirely sure either, copied it from the byte variant

Yep, can't make sense of it anymore. I will have to dig up what the test used to be.

I think the intention was to look for (default, x) and (x, default) in a span containing only default, and assert we always find it at the first index.

Therefore, the inner loop is unnecessary, at least the way the test is written atm:
for (int i = 0; i < length; i++)

It should just be single check. The test should look like this:

for (int length = 0; length < byte.MaxValue; length++) { byte[] a = new byte[length]; Span<byte> span = new Span<byte>(a); int idx = span.IndexOfAny(default, 99); Assert.Equal(0, idx); idx = span.IndexOfAny(99, default); Assert.Equal(0, idx); }

We can leave it as is for now and fix all the tests (byte/char/etc.) in one go outside of this PR.

Edit:

I will have to dig up what the test used to be.

Found where it originated: #17472
The loop didn't make sense then either. My recommendation above holds.

benaadams · 2018-09-03T14:17:38Z

@dotnet-bot test UWP CoreCLR x64 Debug Build

benaadams · 2018-09-03T16:34:29Z

Raised issue for UWP failure https://github.com/dotnet/corefx/issues/32083

danmoseley · 2018-09-13T17:19:12Z

@ahsonkhan is this good to merge now?

benaadams · 2018-09-13T19:42:03Z

@danmosemsft you triggered a rebuild with your comment, looks like infra not happy

10:36:10 Done building Native components
10:36:20 -- The C compiler identification is MSVC 19.14.26433.0
10:36:22 -- The CXX compiler identification is MSVC 19.14.26433.0
10:36:22 -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2017/Enterprise/VC/Tools/MSVC/14.14.26428/bin/Hostx86/x86/cl.exe
10:36:23 Cannot contact windows.10.amd64.clientrs4.devex.open-0913102446322-1: hudson.remoting.RequestAbortedException: java.nio.channels.ClosedChannelException
12:24:57 Cancelling nested steps due to timeout
12:24:57 Could not connect to windows.10.amd64.clientrs4.devex.open-0913102446322-1 to send interrupt signal to process
[Pipeline] }
[Pipeline] // stage
[Pipeline] }
[Pipeline] // timeout
[Pipeline] }
[Pipeline] // timestamps
[Pipeline] echo
Archiving logs in netci-archived-logs/**
[Pipeline] step
Required context class hudson.Launcher is missing
Perhaps you forgot to surround the code with a step that provides this, such as: node
[Pipeline] echo
Error during cleanup: java.lang.IllegalArgumentException: Failed to prepare archiveArtifacts step
[Pipeline] isUnix
Required context class hudson.Launcher is missing
Perhaps you forgot to surround the code with a step that provides this, such as: node
[Pipeline] echo
Some files could not be cleaned up because of running processes.  These processes will be killed immediately and cleanup will happen before the node upon node-reuse
[Pipeline] }
[Pipeline] // node
[Pipeline] End of Pipeline
[BFA] Scanning build for known causes...
[BFA] No failure causes found
[BFA] Done. 0s
Setting status of 658a5716713374ceaed027b4970b4876a04c125f to FAILURE with url https://ci3.dot.net/job/dotnet_corefx/job/master/job/windows-TGroup_netcoreapp+CGroup_Release+AGroup_x86+TestOuter_false_prtest/16424/ and message: 'Build finished. '
Using context: Windows x86 Release Build
Timeout has been exceeded
Finished: ABORTED

benaadams · 2018-09-15T00:30:56Z

@dotnet-bot test NETFX x86 Release Build
@dotnet-bot test Windows x86 Release Build

danmoseley · 2018-09-15T00:33:37Z

@mmitche why do random comments sometimes trigger rebuilds?

mmitche · 2018-09-17T14:45:15Z

@danmosemsft The comment triggers a jenkins webhook, which causes it to compare the last built sha against the current sha. That may not be up to date, and it may cause a rebuild. Jenkins also forgets that info over time (as builds are deleted, restarts, etc.) which makes a rebuild for an old PR more likely.

ahsonkhan · 2018-09-20T03:41:25Z

is this good to merge now?

I haven't gotten a chance to fully review all the tests yet, but does this PR need to wait for dotnet/coreclr#19790 to get merged first?

benaadams · 2018-09-20T16:16:32Z

does this PR need to wait for dotnet/coreclr#19790 to get merged first?

No; its just a copying of the byte tests to char to help validate dotnet/coreclr#19790; so before would probably be better :)

ViktorHofer · 2018-09-25T10:40:33Z

@dotnet-bot test this please

stephentoub · 2018-09-25T15:40:57Z

            byte[] searchForBytes = Encoding.UTF8.GetBytes(searchFor);

-            var index = -1;
+            var index = span.IndexOfAny(new ReadOnlySpan<byte>(searchForBytes));


Nit: var => int (I realize it was like that before... but it shouldn't have been :)

stephentoub · 2018-09-25T22:13:02Z

        {
            byte[] buffers = Encoding.UTF8.GetBytes(raw);
-            var span = new ReadOnlySpan<byte>(buffers);
+            ReadOnlySpan<byte> span = new ReadOnlySpan<byte>(buffers);


FWIW, this was fine as var. Our guidelines just outlaw it when the type isn't named on the RHS, which generally means something other than a new or a cast. But naming it in such situations is fine, too.

stephentoub · 2018-09-25T22:13:44Z

@ahsonkhan, anything more to do here?

benaadams · 2018-09-25T22:14:29Z

UWP infra issue

NuGet.targets(114,5): error : Failed to download package 'System.Linq.4.1.0' from 'https://api.nuget.org/v3-flatcontainer/system.linq/4.1.0/system.linq.4.1.0.nupkg'. [D:\j\workspace\windows-TGrou---c60886e1\external\test-runtime\XUnit.Runtime.depproj] [D:\j\workspace\windows-TGrou---c60886e1\external\test-runtime\XUnit.Runtime.depproj]
NuGet.targets(114,5): error : Response status code does not indicate success: 504 (Gateway Timeout). [D:\j\workspace\windows-TGrou---c60886e1\external\test-runtime\XUnit.Runtime.depproj] [D:\j\workspace\windows-TGrou---c60886e1\external\test-runtime\XUnit.Runtime.depproj]

benaadams · 2018-09-25T22:15:09Z

@dotnet-bot test UWP NETNative x86 Release Build

ahsonkhan

LGTM

* Add specific tests for {RO}Span.IndexOfAny<char> * Always test using IndexOfAny(ROS) * No var * Less var Commit migrated from dotnet/corefx@bfcfd34

Add specific tests for {RO}Span.IndexOfAny<char>

b96331a

benaadams mentioned this pull request Sep 1, 2018

Additionally Vectorize string.IndexOfAny for value lengths 2,3,4,5 dotnet/coreclr#19790

Merged

krwq reviewed Sep 2, 2018

View reviewed changes

Comment thread src/System.Memory/tests/ReadOnlySpan/IndexOfAny.char.cs Outdated

krwq reviewed Sep 2, 2018

View reviewed changes

Always test using IndexOfAny(ROS)

d317fb0

ahsonkhan reviewed Sep 4, 2018

View reviewed changes

Comment thread src/System.Memory/tests/ReadOnlySpan/IndexOfAny.char.cs Outdated

ahsonkhan reviewed Sep 4, 2018

View reviewed changes

Comment thread src/System.Memory/tests/ReadOnlySpan/IndexOfAny.char.cs Outdated

karelz added the area-System.Memory label Sep 6, 2018

karelz assigned benaadams, ahsonkhan and KrzysztofCwalina Sep 6, 2018

karelz added this to the 3.0 milestone Sep 6, 2018

No var

658a571

stephentoub reviewed Sep 25, 2018

View reviewed changes

Less var

762c273

stephentoub reviewed Sep 25, 2018

View reviewed changes

ahsonkhan approved these changes Sep 26, 2018

View reviewed changes

stephentoub merged commit bfcfd34 into dotnet:master Sep 26, 2018

benaadams deleted the span-IndexOfAny-char--Tests branch September 26, 2018 01:25

Conversation

benaadams commented Sep 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

krwq Sep 2, 2018

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 2, 2018

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benaadams commented Sep 3, 2018

Uh oh!

benaadams commented Sep 3, 2018

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Sep 13, 2018

Uh oh!

benaadams commented Sep 13, 2018

Uh oh!

benaadams commented Sep 15, 2018

Uh oh!

danmoseley commented Sep 15, 2018

Uh oh!

mmitche commented Sep 17, 2018

Uh oh!

ahsonkhan commented Sep 20, 2018

Uh oh!

benaadams commented Sep 20, 2018

Uh oh!

ViktorHofer commented Sep 25, 2018

Uh oh!

stephentoub Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

benaadams Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub Sep 25, 2018

Choose a reason for hiding this comment

Uh oh!

stephentoub commented Sep 25, 2018

Uh oh!

benaadams commented Sep 25, 2018

Uh oh!

benaadams commented Sep 25, 2018

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

benaadams commented Sep 1, 2018 •

edited

Loading

ahsonkhan Sep 20, 2018 •

edited

Loading