Skip to content
This repository has been archived by the owner on Aug 8, 2024. It is now read-only.

Commit

Permalink
Fix LargeArrayBuilder.CopyTo returning incorrect end-of-copy position (…
Browse files Browse the repository at this point in the history 
…dotnet#23817)

* Fix #23680

* PR Feedback

* More tests

* More tests
  • Loading branch information
@OmarTawfik @marek-safar
OmarTawfik authored and marek-safar committed Jan 19, 2018
1 parent cb1b049 commit fc70ae3
Show file tree
Hide file tree
Showing 4 changed files with 277 additions and 19 deletions.
57 changes: 41 additions & 16 deletions src/Common/src/System/Collections/Generic/LargeArrayBuilder.cs
View file
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)
{
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)
{
Debug.Assert(sourceBuffer.Length > sourceIndex);

// 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
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)
{
// Finish copying after the final marker.
_builder.CopyTo(position, array, arrayIndex, count);
}
}

/// <summary>
Expand Down
118 changes: 118 additions & 0 deletions src/System.Linq/tests/ConcatTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,5 +421,123 @@ 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

IEnumerable<int> concats = arrays[0];

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

int[] results = concats.ToArray();

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

private static IEnumerable<object[]> GetToArrayDataSources()
{
// 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 },
}
};

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

// Marker in middle
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
}
};

// Non-marker in middle
yield return new object[]
{
new IEnumerable<int>[]
{
new int[] { 0 },
new TestEnumerable<int>(new int[] { 1 }),
new int[] { 2 },
}
};

// Big arrays (marker in middle)
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()),
}
};

// Big arrays (non-marker in middle)
yield return new object[]
{
new IEnumerable<int>[]
{
Enumerable.Range(0, 100).ToArray(),
new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
Enumerable.Range(200, 100).ToArray(),
}
};

// 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 }),
}
};
}
}
}
111 changes: 111 additions & 0 deletions src/System.Linq/tests/SelectManyTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -477,5 +477,116 @@ 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

int[] results = arrays.SelectMany(ar => ar).ToArray();

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

private static IEnumerable<object[]> GetToArrayDataSources()
{
// 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 },
}
};

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

// Marker in middle
yield return new object[]
{
new IEnumerable<int>[]
{
new TestEnumerable<int>(new int[] { 0 }),
new int[] { 1 },
new TestEnumerable<int>(new int[] { 2 }),
}
};

// Non-marker in middle
yield return new object[]
{
new IEnumerable<int>[]
{
new int[] { 0 },
new TestEnumerable<int>(new int[] { 1 }),
new int[] { 2 },
}
};

// Big arrays (marker in middle)
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()),
}
};

// Big arrays (non-marker in middle)
yield return new object[]
{
new IEnumerable<int>[]
{
Enumerable.Range(0, 100).ToArray(),
new TestEnumerable<int>(Enumerable.Range(100, 100).ToArray()),
Enumerable.Range(200, 100).ToArray(),
}
};

// 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 }),
}
};
}
}
}

0 comments on commit fc70ae3

Please sign in to comment.