Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23817

Merged
merged 4 commits into from
Sep 7, 2017
Merged

Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position #23817

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ffe169c
Fix #23680
OmarTawfik Sep 5, 2017
1344a6c
PR Feedback
OmarTawfik Sep 6, 2017
bcc79be
More tests
OmarTawfik Sep 7, 2017
ff2c794
More tests
OmarTawfik Sep 7, 2017
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
57 changes: 41 additions & 16 deletions src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,20 @@ internal CopyPosition(int row, int column)
/// </summary>
internal int Column { get; }

/// <summary>
/// If this position is at the end of the current buffer, returns the position
/// at the start of the next buffer. Otherwise, returns this position.
/// </summary>
/// <param name="endColumn">The length of the current buffer.</param>
public CopyPosition Normalize(int endColumn)
{
Debug.Assert(Column <= endColumn);

return Column == endColumn ?
new CopyPosition(Row + 1, 0) :
this;
}

/// <summary>
/// Gets a string suitable for display in the debugger.
/// </summary>
Expand Down Expand Up @@ -197,9 +211,10 @@ public void CopyTo(T[] array, int arrayIndex, int count)
/// <returns>The position in this builder that was copied up to.</returns>
public CopyPosition CopyTo(CopyPosition position, T[] array, int arrayIndex, int count)
{
Debug.Assert(array != null);
Debug.Assert(arrayIndex >= 0);
Debug.Assert(count >= 0 && count <= Count);
Debug.Assert(array?.Length - arrayIndex >= count);
Debug.Assert(count > 0 && count <= Count);
Debug.Assert(array.Length - arrayIndex >= count);

// Go through each buffer, which contains one 'row' of items.
// The index in each buffer is referred to as the 'column'.
Expand All @@ -216,25 +231,35 @@ public CopyPosition CopyTo(CopyPosition position, T[] array, int arrayIndex, int
int row = position.Row;
int column = position.Column;

for (; count > 0; row++, column = 0)
T[] buffer = GetBuffer(row);
int copied = CopyToCore(buffer, column);

if(count == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: in case you end up making other edits, there's a missing space between if and (

Copy link
Member

Choose a reason for hiding this comment

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

Also, FWIW, I find it very confusing that CopyToCore is capturing count and modifying it. There's no indication at the call site that's happening, which means it looks like the only way count could be 0 is if it was passed in as 0, and yet there's a debug assert for that above.

{
T[] buffer = GetBuffer(index: row);
return new CopyPosition(row, column + copied).Normalize(buffer.Length);
}

// During this iteration, copy until we satisfy `count` or reach the
// end of the current buffer.
int copyCount = Math.Min(buffer.Length, count);
do
{
buffer = GetBuffer(++row);
copied = CopyToCore(buffer, 0);
} while (count > 0);

if (copyCount > 0)
{
Array.Copy(buffer, column, array, arrayIndex, copyCount);
return new CopyPosition(row, copied).Normalize(buffer.Length);

arrayIndex += copyCount;
count -= copyCount;
column += copyCount;
}
}
int CopyToCore(T[] sourceBuffer, int sourceIndex)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Is it customary in corefx to put a local func at the very end of the containing method?

Copy link
Member

Choose a reason for hiding this comment

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

We have very few local methods; there's no convention yet. But unless there's an obviously better place, end of the method makes sense.

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'm not sure what is the custom for this repo. I've not seen a lot of local functions used here yet. But that pattern is common.

Copy link
Member

Choose a reason for hiding this comment

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

I think this could be simpler without a local function, but this will work too.

{
Debug.Assert(sourceBuffer.Length > sourceIndex);
Copy link
Contributor

Choose a reason for hiding this comment

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

You mentioned previously this assert failed. It's good to see that's no longer the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because of the if (count > 0) check added to the calling method.


// Copy until we satisfy `count` or reach the end of the current buffer.
int copyCount = Math.Min(sourceBuffer.Length - sourceIndex, count);
Array.Copy(sourceBuffer, sourceIndex, array, arrayIndex, copyCount);

return new CopyPosition(row: row, column: column);
arrayIndex += copyCount;
count -= copyCount;

return copyCount;
}
}

/// <summary>
Expand Down
10 changes: 7 additions & 3 deletions src/Common/src/System/Collections/Generic/SparseArrayBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,10 @@ public SparseArrayBuilder(bool initialize)
/// <param name="count">The number of items to copy.</param>
public void CopyTo(T[] array, int arrayIndex, int count)
{
Debug.Assert(array != null);
Debug.Assert(arrayIndex >= 0);
Debug.Assert(count >= 0 && count <= Count);
Debug.Assert(array?.Length - arrayIndex >= count);
Debug.Assert(array.Length - arrayIndex >= count);

int copied = 0;
var position = CopyPosition.Start;
Expand Down Expand Up @@ -149,8 +150,11 @@ public void CopyTo(T[] array, int arrayIndex, int count)
count -= reservedCount;
}

// Finish copying after the final marker.
_builder.CopyTo(position, array, arrayIndex, count);
if (count > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is beneficial for scenarios when we insert a marker at the very end, e.g. nonCollection.Append().Append()./* a bunch of times */ or nonCollection.Concat(collection), so 👍

{
// Finish copying after the final marker.
_builder.CopyTo(position, array, arrayIndex, count);
}
}

/// <summary>
Expand Down
84 changes: 84 additions & 0 deletions src/System.Linq/tests/ConcatTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,5 +421,89 @@ public void GetEnumerableOfConcatCollectionChainFollowedByEnumerableNodeShouldBe
Assert.Equal(0xf00, en.Current);
}
}

[Theory]
[MemberData(nameof(GetToArrayDataSources))]
public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
{
// See https://github.com/dotnet/corefx/issues/23680

var concats = arrays[0];
Copy link
Member

Choose a reason for hiding this comment

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

Nit: we try to use var only when the type is obvious from the right hand side, which it isn't here or below


for (int i = 1; i < arrays.Length; i++)
{
concats = concats.Concat(arrays[i]);
}

var results = concats.ToArray();

for (int i = 0; i < results.Length; i++)
{
Assert.Equal(i, results[i]);
}
}

private static IEnumerable<object[]> GetToArrayDataSources()
{
// Case of small arrays (same LAB row)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
}
};

// Case of large arrays (many LAB rows)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
Enumerable.Range(100, 100).ToArray(),
new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
}
};

// Marker at the end
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new TestEnumerable<int>(new int[] { 1 }),
new TestEnumerable<int>(new int[] { 2 }),
new int[] { 3 },
}
};

// Interleaved (first marker)
yield return new object[]
{
new IEnumerable<int>[]
{
new int[] { 0 },
new TestEnumerable<int>(new int[] { 1 }),
new int[] { 2 },
new TestEnumerable<int>(new int[] { 3 }),
new int[] { 4 },
}
};

// Interleaved (first non-marker)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
new int[] { 3 },
new TestEnumerable<int>(new int[] { 4 }),
}
};
}
}
}
77 changes: 77 additions & 0 deletions src/System.Linq/tests/SelectManyTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -477,5 +477,82 @@ public void ThrowOverflowExceptionOnConstituentLargeCounts(int[] counts)
IEnumerable<int> iterator = counts.SelectMany(c => Enumerable.Range(1, c));
Assert.Throws<OverflowException>(() => iterator.Count());
}

[Theory]
[MemberData(nameof(GetToArrayDataSources))]
public void CollectionInterleavedWithLazyEnumerables_ToArray(IEnumerable<int>[] arrays)
{
// See https://github.com/dotnet/corefx/issues/23680

var results = arrays.SelectMany(ar => ar).ToArray();

for (int i = 0; i < results.Length; i++)
{
Assert.Equal(i, results[i]);
}
}

private static IEnumerable<object[]> GetToArrayDataSources()
{
// Case of small arrays (same LAB row)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
}
};

// Case of large arrays (many LAB rows)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(Enumerable.Range(0, 100).ToArray()),
Enumerable.Range(100, 100).ToArray(),
new TestEnumerable<int>(Enumerable.Range(200, 100).ToArray()),
}
};

// Marker at the end
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new TestEnumerable<int>(new int[] { 1 }),
new TestEnumerable<int>(new int[] { 2 }),
new int[] { 3 },
}
};

// Interleaved (first marker)
yield return new object[]
{
new IEnumerable<int>[]
{
new int[] { 0 },
new TestEnumerable<int>(new int[] { 1 }),
new int[] { 2 },
new TestEnumerable<int>(new int[] { 3 }),
new int[] { 4 },
}
};

// Interleaved (first non-marker)
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
new int[] { 3 },
new TestEnumerable<int>(new int[] { 4 }),
}
};
}
}
}