Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
4 changes: 4 additions & 0 deletions src/libraries/System.Memory/ref/System.Memory.cs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@ public static partial class MemoryExtensions
public static int BinarySearch<T, TComparer>(this System.Span<T> span, T value, TComparer comparer) where TComparer : System.Collections.Generic.IComparer<T>, allows ref struct { throw null; }
[System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(-1)]
public static int BinarySearch<T, TComparable>(this System.Span<T> span, TComparable comparable) where TComparable : System.IComparable<T>, allows ref struct { throw null; }
public static T? Max<T>(this System.ReadOnlySpan<T> span) { throw null; }
public static T? Max<T>(this System.ReadOnlySpan<T> span, System.Collections.Generic.IComparer<T> comparer) { throw null; }
public static T? Min<T>(this System.ReadOnlySpan<T> span) { throw null; }
public static T? Min<T>(this System.ReadOnlySpan<T> span, System.Collections.Generic.IComparer<T> comparer) { throw null; }
[System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(-1)]
Comment thread
manandre marked this conversation as resolved.
Outdated
Comment thread
manandre marked this conversation as resolved.
Outdated
public static int CommonPrefixLength<T>(this System.Span<T> span, System.ReadOnlySpan<T> other) { throw null; }
[System.Runtime.CompilerServices.OverloadResolutionPriorityAttribute(-1)]
Expand Down
90 changes: 90 additions & 0 deletions src/libraries/System.Memory/tests/ReadOnlySpan/MinMax.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Generic;
using Xunit;

namespace System.SpanTests
{
public static partial class ReadOnlySpanTests
{
[Fact]
public static void MinMax_Empty_NonNullableValueType_Throws()
{
ReadOnlySpan<int> span = ReadOnlySpan<int>.Empty;

TestHelpers.AssertThrows<InvalidOperationException, int>(span, (_span) => _span.Min());
Comment thread
manandre marked this conversation as resolved.
Outdated
TestHelpers.AssertThrows<InvalidOperationException, int>(span, (_span) => _span.Max());
Comment thread
manandre marked this conversation as resolved.
Outdated
TestHelpers.AssertThrows<InvalidOperationException, int>(span, (_span) => _span.Min(Comparer<int>.Default));
TestHelpers.AssertThrows<InvalidOperationException, int>(span, (_span) => _span.Max(Comparer<int>.Default));
}

[Fact]
public static void MinMax_NullComparer_ThrowsArgumentNullException()
{
ReadOnlySpan<int> span = new int[] { 4, -1, 7, 2 };

TestHelpers.AssertThrows<ArgumentNullException, int>(span, (_span) => _span.Min(comparer: null!));
TestHelpers.AssertThrows<ArgumentNullException, int>(span, (_span) => _span.Max(comparer: null!));
Comment thread
manandre marked this conversation as resolved.
Outdated
}

[Fact]
public static void MinMax_Empty_ReferenceAndNullableValueType_ReturnsNull()
{
ReadOnlySpan<string?> strings = ReadOnlySpan<string?>.Empty;
ReadOnlySpan<int?> nullableInts = ReadOnlySpan<int?>.Empty;

Assert.Null(strings.Min());
Assert.Null(strings.Max());
Assert.Null(nullableInts.Min());
Assert.Null(nullableInts.Max());
}
Comment thread
manandre marked this conversation as resolved.
Comment thread
manandre marked this conversation as resolved.

[Fact]
public static void MinMax_AllNull_ReturnsNull()
{
ReadOnlySpan<string?> strings = new string?[] { null, null, null };
ReadOnlySpan<int?> nullableInts = new int?[] { null, null, null };

Assert.Null(strings.Min());
Assert.Null(strings.Max());
Assert.Null(nullableInts.Min());
Assert.Null(nullableInts.Max());
}

[Fact]
public static void MinMax_NullNotFirst_NullIgnoredForComparison()
{
ReadOnlySpan<string?> strings = new string?[] { "charlie", null, "bravo", null, "delta" };
ReadOnlySpan<int?> nullableInts = new int?[] { 4, null, -1, null, 7 };

Assert.Equal("bravo", strings.Min());
Assert.Equal("delta", strings.Max());
Assert.Equal(-1, nullableInts.Min());
Assert.Equal(7, nullableInts.Max());
}

[Fact]
public static void MinMax_DefaultComparer_ProducesExpectedValues()
Comment thread
manandre marked this conversation as resolved.
{
ReadOnlySpan<int> ints = new int[] { 4, -1, 7, 2 };
ReadOnlySpan<string?> strings = new string?[] { null, "charlie", "bravo", null, "delta" };

Assert.Equal(-1, ints.Min());
Assert.Equal(7, ints.Max());

Assert.Equal("bravo", strings.Min());
Assert.Equal("delta", strings.Max());
}

[Fact]
public static void MinMax_CustomComparer_IsUsed()
{
ReadOnlySpan<int> ints = new int[] { 4, -1, 7, 2 };
IComparer<int> reverse = Comparer<int>.Create((left, right) => right.CompareTo(left));
Comment thread
manandre marked this conversation as resolved.

Assert.Equal(7, ints.Min(reverse));
Assert.Equal(-1, ints.Max(reverse));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
<Compile Include="ParsersAndFormatters\Formatter\RealFormatterTests.cs" />
<Compile Include="$(CommonPath)..\tests\System\RealParserTestsBase.cs" Link="ParsersAndFormatters\Parser\RealParserTestsBase.cs" />
<Compile Include="ParsersAndFormatters\Parser\RealParserTests.cs" />
<Compile Include="ReadOnlySpan\MinMax.cs" />
<Compile Include="ReadOnlySpan\Comparers.cs" />
<Compile Include="ReadOnlySpan\Contains.byte.cs" />
<Compile Include="ReadOnlySpan\Contains.T.cs" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4486,4 +4486,7 @@
<data name="Arg_HexBinaryStylesNotSupported" xml:space="preserve">
<value>The number styles AllowHexSpecifier and AllowBinarySpecifier are not supported on the decimal type.</value>
</data>
<data name="InvalidOperation_NoElements" xml:space="preserve">
<value>Sequence contains no elements</value>
Comment thread
manandre marked this conversation as resolved.
Outdated
</data>
</root>
116 changes: 116 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4312,6 +4312,122 @@ public static int BinarySearch<T, TComparer>(
return BinarySearch(span, comparable);
}

/// <summary>
/// Returns the minimum value in the span.
/// </summary>
/// <typeparam name="T">The type of the elements in the span.</typeparam>
/// <param name="span">The span of values to determine the minimum value of.</param>
/// <returns>The minimum value in the span.</returns>
/// <exception cref="InvalidOperationException"><paramref name="span"/> is empty and <typeparamref name="T"/> is a non-nullable value type.</exception>
Comment thread
manandre marked this conversation as resolved.
Outdated
Comment thread
manandre marked this conversation as resolved.
Outdated
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T? Min<T>(this ReadOnlySpan<T> span) =>
Min(span, Comparer<T>.Default);
Comment thread
manandre marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Much like with LINQ, this should be passing comparer: null and then we should be treating a null comparer as selecting the default, not throwing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@tannergooding Should we then update the approved API to explicitly accept a null comparer?

    public T? Min<T>(this ReadOnlySpan<T> span, IComparer<T>? comparer);
    public T? Max<T>(this ReadOnlySpan<T> span, IComparer<T>? comparer);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nullability annotations are often overlooked in the API review. IComparer<T> parameters is a case that we almost universally allow to be nullable (much like IFormattable, IEqualityComparer, and a couple others) and where we often explicitly want it to be so we can do specific optimizations.

I've left an explicit comment indicating the oversight, so please adjust the PR accordingly.


Comment thread
tannergooding marked this conversation as resolved.
Outdated
Comment thread
manandre marked this conversation as resolved.
Outdated
/// <summary>
/// Returns the minimum value in the span.
/// </summary>
/// <typeparam name="T">The type of the elements in the span.</typeparam>
/// <param name="span">The span of values to determine the minimum value of.</param>
/// <param name="comparer">The <see cref="IComparer{T}"/> to compare values.</param>
/// <returns>The minimum value in the span.</returns>
/// <exception cref="InvalidOperationException"><paramref name="span"/> is empty and <typeparamref name="T"/> is a non-nullable value type.</exception>
public static T? Min<T>(this ReadOnlySpan<T> span, IComparer<T> comparer)
Comment thread
manandre marked this conversation as resolved.
Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's unclear why this deviates from the enumerable logic in implementation.

I'd generally expect the two entry points to be nearly identical. Covering the acceleration and other checks like: https://github.com/dotnet/runtime/blob/main/src/libraries/System.Linq/src/System/Linq/Min.cs#L292-L373

The biggest difference should be that if (!enumerator.MoveNext()) becomes if (i >= span.Length). The JIT should already know that span.Length can't be negative and should know the same of i since we initialize it to 0 and only ever do ++

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have aligned the implementation on the enumerable logic.

{
if (comparer is null)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparer);
Comment thread
manandre marked this conversation as resolved.
Outdated

if (span.IsEmpty)
{
if (default(T) is null)
{
return default;
}

ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NoElements);
}

T? value = span[0];
int i = 1;

while (value is null)
{
if ((uint)i >= (uint)span.Length)
{
return value;
}
value = span[i++];
}

for (; (uint)i < (uint)span.Length; i++)
{
T next = span[i];
if (next is not null && comparer.Compare(next, value) < 0)
{
value = next;
}
}
Comment thread
manandre marked this conversation as resolved.
Outdated
Comment thread
manandre marked this conversation as resolved.
Outdated

return value;
}

/// <summary>
/// Returns the maximum value in the span.
/// </summary>
/// <typeparam name="T">The type of the elements in the span.</typeparam>
/// <param name="span">The span of values to determine the maximum value of.</param>
/// <returns>The maximum value in the span.</returns>
/// <exception cref="InvalidOperationException"><paramref name="span"/> is empty and <typeparamref name="T"/> is a non-nullable value type.</exception>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
public static T? Max<T>(this ReadOnlySpan<T> span) =>
Max(span, Comparer<T>.Default);

/// <summary>
/// Returns the maximum value in the span.
/// </summary>
/// <typeparam name="T">The type of the elements in the span.</typeparam>
/// <param name="span">The span of values to determine the maximum value of.</param>
/// <param name="comparer">The <see cref="IComparer{T}"/> to compare values.</param>
/// <returns>The maximum value in the span.</returns>
/// <exception cref="InvalidOperationException"><paramref name="span"/> is empty and <typeparamref name="T"/> is a non-nullable value type.</exception>
public static T? Max<T>(this ReadOnlySpan<T> span, IComparer<T> comparer)
{
if (comparer is null)
ThrowHelper.ThrowArgumentNullException(ExceptionArgument.comparer);

if (span.IsEmpty)
{
if (default(T) is null)
{
return default;
}

ThrowHelper.ThrowInvalidOperationException(ExceptionResource.InvalidOperation_NoElements);
}

T? value = span[0];
int i = 1;

while (value is null)
{
if ((uint)i >= (uint)span.Length)
{
return value;
}
value = span[i++];
}

for (; (uint)i < (uint)span.Length; i++)
{
T next = span[i];
if (next is not null && comparer.Compare(next, value) > 0)
{
value = next;
}
}

return value;
}

/// <summary>
/// Sorts the elements in the entire <see cref="Span{T}" /> using the <see cref="IComparable{T}" /> implementation
/// of each element of the <see cref= "Span{T}" />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1252,6 +1252,8 @@ private static string GetResourceString(ExceptionResource resource)
return SR.ConcurrentDictionary_ItemKeyIsNull;
case ExceptionResource.ConcurrentDictionary_TypeOfValueIncorrect:
return SR.ConcurrentDictionary_TypeOfValueIncorrect;
case ExceptionResource.InvalidOperation_NoElements:
return SR.InvalidOperation_NoElements;
default:
Debug.Fail("The enum value is not defined, please check the ExceptionResource Enum.");
return "";
Expand Down Expand Up @@ -1453,5 +1455,6 @@ internal enum ExceptionResource
InvalidOperation_IncompatibleComparer,
ConcurrentDictionary_ItemKeyIsNull,
ConcurrentDictionary_TypeOfValueIncorrect,
InvalidOperation_NoElements,
}
}
Loading