Skip to content

Don't use readonly struct in enumerator #74

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
benaadams wants to merge 2 commits into from
Closed

Don't use readonly struct in enumerator #74

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
1c1e21b
Don't use readonly struct in enumerator
benaadams Dec 24, 2015
adcbdff
Faster Count
benaadams Dec 29, 2015
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
41 changes: 18 additions & 23 deletions src/Microsoft.Extensions.Primitives/StringValues.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public static implicit operator string[] (StringValues value)
return value.GetArrayValue();
}

public int Count => _values?.Length ?? (_value != null ? 1 : 0);
public int Count => _value != null ? 1 : (_values?.Length ?? 0);

bool ICollection<string>.IsReadOnly
{
Expand Down Expand Up @@ -206,7 +206,7 @@ void ICollection<string>.Clear()

public Enumerator GetEnumerator()
{
return new Enumerator(this);
return new Enumerator(ref this);
}

IEnumerator<string> IEnumerable<string>.GetEnumerator()
Expand Down Expand Up @@ -420,43 +420,39 @@ public override int GetHashCode()

public struct Enumerator : IEnumerator<string>
{
private readonly StringValues _values;
private readonly string[] _values;
private string _current;
private int _index;

public Enumerator(StringValues values)
public Enumerator(ref StringValues values)
{
_values = values;
_current = null;
_values = values._values;
_current = values._value;
Copy link
Copy Markdown
Member

@rynowak rynowak Dec 28, 2015

Choose a reason for hiding this comment

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

When is this ever correct? Why not just leave it null, which is equally 'undefined'

Copy link
Copy Markdown
Member Author

@benaadams benaadams Dec 28, 2015

Choose a reason for hiding this comment

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

StringValue with 0 and 1 count; where the array is not set. Changed move next to fit the behaviour.

Copy link
Copy Markdown
Member

@rynowak rynowak Dec 28, 2015

Choose a reason for hiding this comment

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

Gotcha

_index = 0;
}

public bool MoveNext()
{
var values = _values._values;
if (values != null)
if (_index < 0)
{
if (_index < values.Length)
return false;
}

if (_values != null)
{
if (_index < _values.Length)
{
_current = values[_index];
_current = _values[_index];
_index++;
return true;
}

_current = null;
_index = -1;
return false;
}

var value = _values._value;
if (value != null && _index == 0)
{
_current = value;
_index = -1; // sentinel value
return true;
}

_current = null;
return false;
_index = -1; // sentinel value
return _current != null;
}

public string Current => _current;
Expand All @@ -465,8 +461,7 @@ public bool MoveNext()

void IEnumerator.Reset()
{
_current = null;
_index = 0;
throw new NotSupportedException();
}

void IDisposable.Dispose()
Expand Down
3 changes: 0 additions & 3 deletions test/Microsoft.Extensions.Primitives.Tests/StringValuesTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,6 @@ public void DefaultNullOrEmpty_Enumerator(StringValues stringValues)
public void Enumerator(StringValues stringValues, string[] expected)
{
var e = stringValues.GetEnumerator();
Assert.Null(e.Current);
for (int i = 0; i < expected.Length; i++)
{
Assert.True(e.MoveNext());
Expand All @@ -249,7 +248,6 @@ public void Enumerator(StringValues stringValues, string[] expected)
Assert.False(e.MoveNext());

var e1 = ((IEnumerable<string>)stringValues).GetEnumerator();
Assert.Null(e1.Current);
for (int i = 0; i < expected.Length; i++)
{
Assert.True(e1.MoveNext());
Expand All @@ -260,7 +258,6 @@ public void Enumerator(StringValues stringValues, string[] expected)
Assert.False(e1.MoveNext());

var e2 = ((IEnumerable)stringValues).GetEnumerator();
Assert.Null(e2.Current);
for (int i = 0; i < expected.Length; i++)
{
Assert.True(e2.MoveNext());
Expand Down