Skip to content
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.

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

API Proposal: MemoryExtensions.SequenceEqual with IComparer<T> #48304

Closed
stephentoub opened this issue Feb 15, 2021 · 5 comments · Fixed by #48677
Closed

API Proposal: MemoryExtensions.SequenceEqual with IComparer<T> #48304

stephentoub opened this issue Feb 15, 2021 · 5 comments · Fixed by #48677
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Feb 15, 2021

Background and Motivation

We currently have MemoryExtensions.SequenceEqual overloads that work with spans, but they constrain T : IEquatable<T>:

public static bool SequenceEqual<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> other) where T : System.IEquatable<T> { throw null; }
public static bool SequenceEqual<T>(this System.Span<T> span, System.ReadOnlySpan<T> other) where T : System.IEquatable<T> { throw null; }

This means there's no built-in support for using IEqualityComparer<T>. It also means APIs like Enumerable.SequenceEqual that are unconstrained aren't able to delegate to this in general when provided with array inputs.

Proposed API

public static class MemoryExtensions
{
    public static bool SequenceEqual<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> other, IEqualityComparer<T>? comparer);
    public static bool SequenceEqual<T>(this System.Span<T> span, System.ReadOnlySpan<T> other, IEqualityComparer<T>? comparer);
    ...
}

We could also consider doing the same thing for SequenceCompareTo, taking the existing overloads and adding new ones that remove the constraint and accept a nullable IComparer<T>.

The implementations would delegate to the existing implementations if comparer is null and T is found to be IEquatable<T> (maybe just for value types). Otherwise, they'd loop through the spans comparing each element.

Usage Examples

For example, we would change Enumerable.SequenceEqual to do:

if (first is TSource[] firstArray && second is TSource[] secondArray)
{
    return ((ReadOnlySequence<TSource>)firstArray).SequenceEqual(secondArray, comparer);
}
@stephentoub stephentoub added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Feb 15, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Feb 15, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Memory untriaged New issue has not been triaged by the area owner labels Feb 15, 2021
@ghost
Copy link

ghost commented Feb 15, 2021

Tagging subscribers to this area: @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and Motivation

We currently have MemoryExtensions.SequenceEqual overloads that work with spans, but they constrain T : IEquatable<T>:

        public static bool SequenceEqual<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> other) where T : System.IEquatable<T> { throw null; }
        public static bool SequenceEqual<T>(this System.Span<T> span, System.ReadOnlySpan<T> other) where T : System.IEquatable<T> { throw null; }

This means there's no built-in support for using IEqualityComparer<T>. It also means APIs like Enumerable.SequenceEqual that are unconstrained aren't able to delegate to this in general when provided with array inputs.

Proposed API

public static class MemoryExtensions
{
    public static bool SequenceEqual<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> other, IEqualityComparer<T>? comparer);
    public static bool SequenceEqual<T>(this System.Span<T> span, System.ReadOnlySpan<T> other, IEqualityComparer<T>? comparer);
    ...
}

We could also consider doing the same thing for SequenceCompareTo, taking the existing overloads and adding new ones that remove the constraint and accept a nullable IComparer<T>.

Usage Examples

For example, we would change Enumerable.SequenceEqual to do:

if (first is TSource[] firstArray && second is TSource[] secondArray)
{
    return ((ReadOnlySequence<TSource>)firstArray).SequenceEqual(secondArray, comparer);
}

<table>
  <tr>
    <th align="left">Author:</th>
    <td>stephentoub</td>
  </tr>
  <tr>
    <th align="left">Assignees:</th>
    <td>-</td>
  </tr>
  <tr>
    <th align="left">Labels:</th>
    <td>

`api-ready-for-review`, `area-System.Memory`, `untriaged`

</td>
  </tr>
  <tr>
    <th align="left">Milestone:</th>
    <td>6.0.0</td>
  </tr>
</table>
</details>

@benaadams
Copy link
Member

benaadams commented Feb 15, 2021

Would it need a IEqualityComparer<T>? comparer = null optional param to be used as your example? (which doesn't specify the comparer)

@stephentoub
Copy link
Member Author

to be used as your example? (which doesn't specify the comparer)

My example does pass a comparer... ?

@benaadams
Copy link
Member

My example does pass a comparer... ?

😅 So it does

@stephentoub stephentoub self-assigned this Feb 23, 2021
@stephentoub stephentoub removed the untriaged New issue has not been triaged by the area owner label Feb 23, 2021
@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 23, 2021
@terrajobst
Copy link
Member

terrajobst commented Feb 23, 2021

Video

  • Makes sense
  • We could drop the constraint on the existing APIs and do the right thing internally, but we can do that later
    • Or we could default the comparer to null, which means the caller will either use the existing one when it is equatable and use the new one when it's not.
namespace System
{
    public static class MemoryExtensions
    {
        // Existing
        // public static bool SequenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other) where T : IEquatable<T>;
        // public static bool SequenceEqual<T>(this Span<T> span, ReadOnlySpan<T> other) where T : System.IEquatable<T>;
        public static bool SequenceEqual<T>(this ReadOnlySpan<T> span, ReadOnlySpan<T> other, IEqualityComparer<T>? comparer = null);
        public static bool SequenceEqual<T>(this Span<T> span, ReadOnlySpan<T> other, IEqualityComparer<T>? comparer = null);
    }
}

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 24, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants