Skip to content

Commit

Permalink
Fixed ReadOnlySpan2D<T>.Slice parameters order (#3951)
Browse files Browse the repository at this point in the history 
## Fixup for #3353 
<!-- Add the relevant issue number after the "#" mentioned above (for ex: Fixes #1234) which will automatically close the issue once the PR is merged. -->

<!-- Add a brief overview here of the feature/bug & fix. -->

## PR Type
What kind of change does this PR introduce?
<!-- Please uncomment one or more that apply to this PR. -->

- Bugfix
<!-- - Feature -->
<!-- - Code style update (formatting) -->
<!-- - Refactoring (no functional changes, no api changes) -->
<!-- - Build or CI related changes -->
<!-- - Documentation content changes -->
<!-- - Sample app changes -->
<!-- - Other... Please describe: -->


## Overview
<!-- Please describe the current behavior that you are modifying, or link to a relevant issue. -->
`ReadOnlySpan2D<T>.Slice` has the incorrect ordering for the `height` and `width` parameters, which is inverted with respect to those in `Span2D<T>.Slice`. This is an oversight from a refactor in the original PR, and as a result it also causes the `ReadOnlySpan2D<T>.this[Range, Range]` indexer to fail for `ReadOnlySpan2D<T>`. This PR fixes both issues and adds a new series of tests for the two indexers as well. We probably didn't notice this before since those indexers are not available on UWP 🤔

> **NOTE:** this PR technically introduces a breaking change (swapped parameters in the `.Slice` method, but as that's an issue and not intended, and also because they're in the right order in `Span2D<T>`, it's more of a fix than an actual breaking change.

## PR Checklist

Please check if your PR fulfills the following requirements:

- [X] Tested code with current [supported SDKs](../readme.md#supported)
- [ ] ~~Pull Request has been submitted to the documentation repository [instructions](..\contributing.md#docs). Link: <!-- docs PR link -->~~
- [ ] ~~Sample in sample app has been added / updated (for bug fixes / features)~~
    - [ ] ~~Icon has been created (if new sample) following the [Thumbnail Style Guide and templates](https://github.com/windows-toolkit/WindowsCommunityToolkit-design-assets)~~
- [X] New major technical changes in the toolkit have or will be added to the [Wiki](https://github.com/windows-toolkit/WindowsCommunityToolkit/wiki) e.g. build changes, source generators, testing infrastructure, sample creation changes, etc...
- [X] Tests for the changes have been added (for bug fixes / features) (if applicable)
- [X] Header has been added to all new source files (run *build/UpdateHeaders.bat*)
- [ ] Contains **NO** breaking changes
  • Loading branch information
msftbot[bot] authored Apr 22, 2021
2 parents c31e049 + 6e40cf8 commit 842e2ff
Show file tree
Hide file tree
Showing 3 changed files with 177 additions and 11 deletions.
12 changes: 6 additions & 6 deletions Microsoft.Toolkit.HighPerformance/Memory/ReadOnlySpan2D{T}.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -803,15 +803,15 @@ public ref T DangerousGetReferenceAt(int i, int j)
/// </summary>
/// <param name="row">The target row to map within the current instance.</param>
/// <param name="column">The target column to map within the current instance.</param>
/// <param name="width">The width to map within the current instance.</param>
/// <param name="height">The height to map within the current instance.</param>
/// <param name="width">The width to map within the current instance.</param>
/// <exception cref="ArgumentException">
/// Thrown when either <paramref name="height"/>, <paramref name="width"/> or <paramref name="height"/>
/// are negative or not within the bounds that are valid for the current instance.
/// </exception>
/// <returns>A new <see cref="ReadOnlySpan2D{T}"/> instance representing a slice of the current one.</returns>
[Pure]
public ReadOnlySpan2D<T> Slice(int row, int column, int width, int height)
public ReadOnlySpan2D<T> Slice(int row, int column, int height, int width)
{
if ((uint)row >= Height)
{
Expand All @@ -823,14 +823,14 @@ public ReadOnlySpan2D<T> Slice(int row, int column, int width, int height)
ThrowHelper.ThrowArgumentOutOfRangeExceptionForColumn();
}

if ((uint)width > (this.width - column))
if ((uint)height > (Height - row))
{
ThrowHelper.ThrowArgumentOutOfRangeExceptionForWidth();
ThrowHelper.ThrowArgumentOutOfRangeExceptionForHeight();
}

if ((uint)height > (Height - row))
if ((uint)width > (this.width - column))
{
ThrowHelper.ThrowArgumentOutOfRangeExceptionForHeight();
ThrowHelper.ThrowArgumentOutOfRangeExceptionForWidth();
}

nint shift = ((nint)(uint)this.stride * (nint)(uint)row) + (nint)(uint)column;
Expand Down
93 changes: 88 additions & 5 deletions UnitTests/UnitTests.HighPerformance.Shared/Memory/Test_ReadOnlySpan2D{T}.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,89 @@ ref Unsafe.AsRef<int>(null),
Assert.IsTrue(Unsafe.AreSame(ref r0, ref array[0, 0]));
}

#if NETCOREAPP3_1_OR_GREATER
[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_ReadOnlySpan2DT_Index_Indexer_1()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);

ref int arrayRef = ref array[1, 3];
ref readonly int span2dRef = ref span2d[1, ^1];

Assert.IsTrue(Unsafe.AreSame(ref arrayRef, ref Unsafe.AsRef(in span2dRef)));
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_ReadOnlySpan2DT_Index_Indexer_2()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);

ref int arrayRef = ref array[2, 1];
ref readonly int span2dRef = ref span2d[^2, ^3];

Assert.IsTrue(Unsafe.AreSame(ref arrayRef, ref Unsafe.AsRef(in span2dRef)));
}

[TestCategory("Span2DT")]
[TestMethod]
[ExpectedException(typeof(IndexOutOfRangeException))]
public unsafe void Test_ReadOnlySpan2DT_Index_Indexer_Fail()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);

ref readonly int span2dRef = ref span2d[^6, 2];
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_ReadOnlySpan2DT_Range_Indexer_1()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);
ReadOnlySpan2D<int> slice = span2d[1.., 1..];

Assert.AreEqual(slice.Length, 9);
Assert.IsTrue(Unsafe.AreSame(ref array[1, 1], ref Unsafe.AsRef(in slice[0, 0])));
Assert.IsTrue(Unsafe.AreSame(ref array[3, 3], ref Unsafe.AsRef(in slice[2, 2])));
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_ReadOnlySpan2DT_Range_Indexer_2()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);
ReadOnlySpan2D<int> slice = span2d[0..^2, 1..^1];

Assert.AreEqual(slice.Length, 4);
Assert.IsTrue(Unsafe.AreSame(ref array[0, 1], ref Unsafe.AsRef(in slice[0, 0])));
Assert.IsTrue(Unsafe.AreSame(ref array[1, 2], ref Unsafe.AsRef(in slice[1, 1])));
}

[TestCategory("Span2DT")]
[TestMethod]
[ExpectedException(typeof(ArgumentOutOfRangeException))]
public unsafe void Test_ReadOnlySpan2DT_Range_Indexer_Fail()
{
int[,] array = new int[4, 4];

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);
_ = span2d[0..6, 2..^1];

Assert.Fail();
}
#endif

[TestCategory("ReadOnlySpan2DT")]
[TestMethod]
public void Test_ReadOnlySpan2DT_Slice_1()
Expand All @@ -412,7 +495,7 @@ public void Test_ReadOnlySpan2DT_Slice_1()

ReadOnlySpan2D<int> span2d = new ReadOnlySpan2D<int>(array);

ReadOnlySpan2D<int> slice1 = span2d.Slice(1, 1, 2, 1);
ReadOnlySpan2D<int> slice1 = span2d.Slice(1, 1, 1, 2);

Assert.AreEqual(slice1.Length, 2);
Assert.AreEqual(slice1.Height, 1);
Expand All @@ -431,11 +514,11 @@ public void Test_ReadOnlySpan2DT_Slice_1()

Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(-1, 1, 1, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, -1, 1, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 1, -1, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 1, 1, -1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 1, -1, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(10, 1, 1, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 12, 12, 1));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 1, 1, 55));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 12, 1, 12));
Assert.ThrowsException<ArgumentOutOfRangeException>(() => new ReadOnlySpan2D<int>(array).Slice(1, 1, 55, 1));
}

[TestCategory("ReadOnlySpan2DT")]
Expand All @@ -458,7 +541,7 @@ public void Test_ReadOnlySpan2DT_Slice_2()
Assert.AreEqual(slice1[0, 0], 1);
Assert.AreEqual(slice1[1, 1], 5);

ReadOnlySpan2D<int> slice2 = slice1.Slice(1, 0, 2, 1);
ReadOnlySpan2D<int> slice2 = slice1.Slice(1, 0, 1, 2);

Assert.AreEqual(slice2.Length, 2);
Assert.AreEqual(slice2.Height, 1);
Expand Down
83 changes: 83 additions & 0 deletions UnitTests/UnitTests.HighPerformance.Shared/Memory/Test_Span2D{T}.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -564,6 +564,89 @@ ref Unsafe.AsRef<int>(null),
Assert.IsTrue(Unsafe.AreSame(ref r0, ref array[0, 0]));
}

#if NETCOREAPP3_1_OR_GREATER
[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_Span2DT_Index_Indexer_1()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);

ref int arrayRef = ref array[1, 3];
ref int span2dRef = ref span2d[1, ^1];

Assert.IsTrue(Unsafe.AreSame(ref arrayRef, ref span2dRef));
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_Span2DT_Index_Indexer_2()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);

ref int arrayRef = ref array[2, 1];
ref int span2dRef = ref span2d[^2, ^3];

Assert.IsTrue(Unsafe.AreSame(ref arrayRef, ref span2dRef));
}

[TestCategory("Span2DT")]
[TestMethod]
[ExpectedException(typeof(IndexOutOfRangeException))]
public unsafe void Test_Span2DT_Index_Indexer_Fail()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);

ref int span2dRef = ref span2d[^6, 2];
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_Span2DT_Range_Indexer_1()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);
Span2D<int> slice = span2d[1.., 1..];

Assert.AreEqual(slice.Length, 9);
Assert.IsTrue(Unsafe.AreSame(ref array[1, 1], ref slice[0, 0]));
Assert.IsTrue(Unsafe.AreSame(ref array[3, 3], ref slice[2, 2]));
}

[TestCategory("Span2DT")]
[TestMethod]
public unsafe void Test_Span2DT_Range_Indexer_2()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);
Span2D<int> slice = span2d[0..^2, 1..^1];

Assert.AreEqual(slice.Length, 4);
Assert.IsTrue(Unsafe.AreSame(ref array[0, 1], ref slice[0, 0]));
Assert.IsTrue(Unsafe.AreSame(ref array[1, 2], ref slice[1, 1]));
}

[TestCategory("Span2DT")]
[TestMethod]
[ExpectedException(typeof(ArgumentOutOfRangeException))]
public unsafe void Test_Span2DT_Range_Indexer_Fail()
{
int[,] array = new int[4, 4];

Span2D<int> span2d = new Span2D<int>(array);
_ = span2d[0..6, 2..^1];

Assert.Fail();
}
#endif

[TestCategory("Span2DT")]
[TestMethod]
public void Test_Span2DT_Slice_1()
Expand Down

0 comments on commit 842e2ff

Please sign in to comment.