Skip to content

Align ROS.Split handling of zero-length target strings with String.Split #37742

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

Closed
layomia wants to merge 2 commits into from
Closed

Align ROS.Split handling of zero-length target strings with String.Split #37742

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
80cffc5
Move expected values of span split tests to first arg position in ass…
layomia Jun 11, 2020
1c40f85
Align ROS.Split handling of zero-length target strings with String.Split
layomia Jun 11, 2020
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
55 changes: 31 additions & 24 deletions src/libraries/System.Memory/tests/ReadOnlySpan/Split.char.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,19 @@ public static void SplitNoMatchSingleResult()
ReadOnlySpan<char> value = "a b";

string expected = value.ToString();
var enumerator = value.Split(',');
SpanSplitEnumerator<char> enumerator = value.Split(',');
Assert.True(enumerator.MoveNext());
Assert.Equal(expected, value[enumerator.Current].ToString());

enumerator = value.Split(",");
Assert.True(enumerator.MoveNext());
Assert.Equal(expected, value[enumerator.Current].ToString());
}

[Fact]
public static void DefaultSpanSplitEnumeratorBehavior()
{
var charSpanEnumerator = new SpanSplitEnumerator<string>();
var charSpanEnumerator = new SpanSplitEnumerator<char>();
Copy link
Contributor

@bbartels bbartels Jun 11, 2020

Choose a reason for hiding this comment

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

Suggested change
var charSpanEnumerator = new SpanSplitEnumerator<char>();
ReadOnlySpan<char> charSpanEnumerator = new SpanSplitEnumerator<char>();

To align with the other changes from var to ReadOnlySpan<T>.
Same below, but for some reason cannot add a suggestion, as it is not part of the diff.

Assert.Equal(new Range(0, 0), charSpanEnumerator.Current);
Assert.False(charSpanEnumerator.MoveNext());

Expand All @@ -41,7 +45,7 @@ public static void ValidateArguments_OverloadWithoutSeparator()
{
ReadOnlySpan<char> buffer = default;

SpanSplitEnumerator<char> enumerator = buffer.Split();
SpanSplitEnumerator<char> enumerator = buffer.Split();
Assert.True(enumerator.MoveNext());
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());
Expand All @@ -67,27 +71,27 @@ public static void ValidateArguments_OverloadWithROSSeparator()
// Default buffer
ReadOnlySpan<char> buffer = default;

SpanSplitEnumerator<char> enumerator = buffer.Split(default(char));
SpanSplitEnumerator<char> enumerator = buffer.Split(default(char));
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split(' ');
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

// Empty buffer
buffer = "";

enumerator = buffer.Split(default(char));
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split(' ');
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

// Single whitespace buffer
Expand All @@ -114,60 +118,55 @@ public static void ValidateArguments_OverloadWithStringSeparator()

SpanSplitEnumerator<char> enumerator = buffer.Split(null); // null is treated as empty string
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split("");
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split(" ");
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

// Empty buffer
buffer = "";

enumerator = buffer.Split(null);
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split("");
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split(" ");
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.False(enumerator.MoveNext());

// Single whitespace buffer
buffer = " ";

enumerator = buffer.Split(null); // null is treated as empty string
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(1, 1));
Assert.Equal(new Range(0, 1), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split("");
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(1, 1));
Assert.Equal(new Range(0, 1), enumerator.Current);
Assert.False(enumerator.MoveNext());

enumerator = buffer.Split(" ");
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(0, 0));
Assert.Equal(new Range(0, 0), enumerator.Current);
Assert.True(enumerator.MoveNext());
Assert.Equal(enumerator.Current, new Range(1, 1));
Assert.Equal(new Range(1, 1), enumerator.Current);
Assert.False(enumerator.MoveNext());
}

Expand Down Expand Up @@ -231,6 +230,14 @@ public static void SpanSplitDefaultCharSeparator(string valueParam, string[] exp
}

[Theory]
[InlineData("a,b,c", null, new[] { "a,b,c" })]
Copy link
Member

@danmoseley danmoseley Jun 11, 2020

Choose a reason for hiding this comment

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

empty string (value) ?

[InlineData("a,b,c", "", new[] { "a,b,c" })]
[InlineData("a,b,c,", null, new[] { "a,b,c," })]
[InlineData("a,b,c,", "", new[] { "a,b,c," })]
[InlineData(" a,b,c,", null, new[] { " a,b,c," })]
[InlineData(" a,b,c,", "", new[] { " a,b,c," })]
[InlineData("aaabaaabaaa", "aa", new[] { "", "ab", "ab", "a" })]
[InlineData("this, is, a, string, with some spaces", ", ", new[] { "this", "is", "a", "string", "with some spaces" })]
[InlineData(" Foo Bar Baz,", ", ", new[] { " Foo Bar Baz," })]
[InlineData(" Foo Bar Baz, ", ", ", new[] { " Foo Bar Baz", "" })]
[InlineData(", Foo Bar Baz, ", ", ", new[] { "", "Foo Bar Baz", "" })]
Expand All @@ -251,7 +258,7 @@ private static void AssertEqual<T>(T[][] items, ReadOnlySpan<T> orig, SpanSplitE
foreach (var item in items)
{
Assert.True(source.MoveNext());
var slice = orig[source.Current];
ReadOnlySpan<T> slice = orig[source.Current];
Assert.Equal(item, slice.ToArray());
}
Assert.False(source.MoveNext());
Expand Down
19 changes: 17 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/SpanSplitEnumerator.T.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ internal SpanSplitEnumerator(ReadOnlySpan<T> span, ReadOnlySpan<T> separators)
_separators = separators;
_separator = default!;
_splitOnSingleToken = false;
_separatorLength = _separators.Length != 0 ? _separators.Length : 1;
_separatorLength = _separators.Length;
_startCurrent = 0;
_endCurrent = 0;
_startNext = 0;
Expand Down Expand Up @@ -76,7 +76,22 @@ public bool MoveNext()
ReadOnlySpan<T> slice = _buffer.Slice(_startNext);
_startCurrent = _startNext;

int separatorIndex = _splitOnSingleToken ? slice.IndexOf(_separator) : slice.IndexOf(_separators);
int separatorIndex;
Copy link
Contributor

@bbartels bbartels Jun 11, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Whats the motivation for this change? To improve runtime perf of Split(null) to make it not O(n^2)?

Oh, nevermind, i did not realize string.Split("") just returns the original string.

if (_splitOnSingleToken)
{
separatorIndex = slice.IndexOf(_separator);
}
else if (_separators.Length > 0)
{
separatorIndex = slice.IndexOf(_separators);
}
else
{
_endCurrent = _startCurrent + slice.Length;
_startNext = _endCurrent + 1;
return true;
}

int elementLength = (separatorIndex != -1 ? separatorIndex : slice.Length);

_endCurrent = _startCurrent + elementLength;
Expand Down