Skip to content
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

Adds SpanSplitEnumerator #104534

Draft
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

bbartels
Copy link
Contributor

@bbartels bbartels commented Jul 7, 2024

Implements #934

TODO:

  • Add SplitAny tests
  • Add SearchValues tests
  • Add documentation for public API

IEquatable<T> type constraint is necessary due to the type constraint existing on the System.MemoryExtensions.IndexOf<T>() overloads and on SearchValues<T>.

Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

1 similar comment
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 7, 2024

namespace System.SpanTests
{
public static partial class ReadOnlySpanTests
Copy link
Member

Choose a reason for hiding this comment

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

Can you make the existing string.Split test cases work for this as well, validating that the new Split produces the same splits as string.Split does?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I may not be understanding completely, do you mean:

  1. Find the test cases for string.Split
  2. Move them into tests/ReadOnlySpan/Split.char.cs
    or just add an Assert to the custom AssertEquals method in this file that does an additional string.Split to ensure the result of the SpanSplitEnumerator and string.Split align?

Copy link
Member

Choose a reason for hiding this comment

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

just add an Assert to the custom AssertEquals method in this file that does an additional string.Split to ensure the result of the SpanSplitEnumerator and string.Split align?

This :)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, no, I misread what you wrote. I meant the equivalent of this:

// These tests validate both String.Split and MemoryExtensions.Split, as they have equivalent semantics, with the
// former creating a new array to store the results and the latter writing the results into a supplied span.

Augmenting those tests to test these overloads as well, rather than duplicating a ton of tests.

@@ -191,6 +191,7 @@
<Compile Include="ReadOnlySpan\SequenceEqual.long.cs" />
<Compile Include="ReadOnlySpan\SequenceEqual.T.cs" />
<Compile Include="ReadOnlySpan\Slice.cs" />
<Compile Include="ReadOnlySpan\Split.char.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

What about T other than char?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PR was still in draft, so was still going to work on more exhaustive tests. What/How many other types do you think would be reasonable to test? Not sure if this is a good idea, but I could generalise the tests so it casts the incoming string of the existing tests to ReadOnlySpan<byte/long/etc>. Though of course this means you are realistically testing the same exact data, just with the types differing.

Copy link
Member

Choose a reason for hiding this comment

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

What/How many other types do you think would be reasonable to test?

Just basic validation of an assortment of other types, e.g. byte, int, a custom reference type, and a custom value type.


private int _startCurrent = 0;
private int _endCurrent = 0;
private int _startNext = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is _startNext needed rather than just updating _span to be the remainder to be searched?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As with https://github.com/dotnet/runtime/pull/104534/files#r1668846871 I believe this was PR feedback at the time. Happy to change it

@bbartels bbartels force-pushed the SpanSplitEnumeratorNewFinalThisTimeForSure branch from 8c52bf2 to dd98d20 Compare July 8, 2024 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Memory community-contribution Indicates that the PR has been added by a community member new-api-needs-documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants