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]: Span.StartsWith/EndsWith(T) #87689

Closed
MihaZupan opened this issue Jun 16, 2023 · 15 comments · Fixed by #103458
Closed

[API Proposal]: Span.StartsWith/EndsWith(T) #87689

MihaZupan opened this issue Jun 16, 2023 · 15 comments · Fixed by #103458
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@MihaZupan
Copy link
Member

MihaZupan commented Jun 16, 2023

Background and motivation

string.StartsWith(char) and string.EndsWith(char) are useful shortcuts for this.Length > 0 && this[0] == 'a'.
They also sometimes save you from having to declare temporary locals SomeCall().StartsWith('a').
For strings, they are also more efficient now but that's not the main motivation and likely wouldn't apply to Span extensions.

The fact that Span is missing such a helper feels like an oversight on an otherwise helpful API surface of utility extensions and I often end up writing a helper class that polyfills these. A quick GH search shows ~10 other such places.

It also means that some analyzers light up on string, but not on ReadOnlySpan<char>, e.g. the recently added CA1858 - "Use StartsWith instead of IndexOf".

I didn't find an existing issue for these so figured they're at least worth discussing.
At least unlike most? other extensions (e.g. IndexOf, StartsWith(ReadOnlySpan, ReadOnlySpan) ...), these methods are trivial to polyfill without losing out on potential perf.

Edit: more discussion around these APIs also at #85374

API Proposal

namespace System;

public static class MemoryExtensions
{
    // Existing
    public static bool StartsWith(this System.ReadOnlySpan<char> span, System.ReadOnlySpan<char> value, System.StringComparison comparisonType);
    public static bool StartsWith<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> value) where T : System.IEquatable<T>?;
    public static bool StartsWith<T>(this System.Span<T> span, System.ReadOnlySpan<T> value) where T : System.IEquatable<T>?;
    // ... same pattern for EndsWith

    // Proposed
    public static bool StartsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool StartsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
}

API Usage

-static bool IsNameHidden(ReadOnlySpan<char> fileName) => fileName.Length > 0 && fileName[0] == '.';
+static bool IsNameHidden(ReadOnlySpan<char> fileName) => fileName.StartsWith('.');

Alternative Designs

While most helpers on MemoryExtensions are available for both Span and ReadOnlySpan of any T, the vast majority of use for this one will likely end up being ReadOnlySpan<char>.

public static bool StartsWith(this System.ReadOnlySpan<char> span, char value);
public static bool EndsWith(this System.ReadOnlySpan<char> span, char value);

Risks

No response

@MihaZupan MihaZupan added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Memory labels Jun 16, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 16, 2023
@ghost
Copy link

ghost commented Jun 16, 2023

Tagging subscribers to this area: @dotnet/area-system-memory
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

string.StartsWith(char) and string.EndsWith(char) are useful shortcuts for this.Length > 0 && this[0] == 'a'.
They also sometimes save you from having to declare temporary locals SomeCall().StartsWith('a').
For strings, they are also more efficient now but that's not the main motivation and likely wouldn't apply to Span extensions.

The fact that Span is missing such a helper feels like an oversight on an otherwise helpful API surface of utility extensions and I often end up writing a helper class that polyfills these. A quick GH search shows ~10 other such places.

It also means that some analyzers light up on string, but not on ReadOnlySpan<char>, e.g. the recently added CA1858 - "Use StartsWith instead of IndexOf".

I didn't find an existing issue for these so figured they're at least worth discussing.
At least unlike most? other extensions (e.g. IndexOf, StartsWith(ReadOnlySpan, ReadOnlySpan) ...), these methods are trivial to polyfill without losing out on potential perf.

API Proposal

namespace System;

public static class MemoryExtensions
{
    // Existing
    public static bool StartsWith(this System.ReadOnlySpan<char> span, System.ReadOnlySpan<char> value, System.StringComparison comparisonType);
    public static bool StartsWith<T>(this System.ReadOnlySpan<T> span, System.ReadOnlySpan<T> value) where T : System.IEquatable<T>?;
    public static bool StartsWith<T>(this System.Span<T> span, System.ReadOnlySpan<T> value) where T : System.IEquatable<T>?;
    // ... same pattern for EndsWith

    // Proposed
    public static bool StartsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool StartsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
}

API Usage

-static bool IsNameHidden(ReadOnlySpan<char> fileName) => fileName.Length > 0 && fileName[0] == '.';
+static bool IsNameHidden(ReadOnlySpan<char> fileName) => fileName.StartsWith('.');

Alternative Designs

While most helpers on MemoryExtensions are available for both Span and ReadOnlySpan of any T, the vast majority of use for this one will likely end up being ReadOnlySpan<char>.

public static bool StartsWith(this System.ReadOnlySpan<char> span, char value);
public static bool EndsWith(this System.ReadOnlySpan<char> span, char value);

Risks

No response

Author: MihaZupan
Assignees: -
Labels:

api-suggestion, area-System.Memory

Milestone: -

@Clockwork-Muse
Copy link
Contributor

Likely these don't exist because you can do:

// For non-reference types
MemoryExtensions.StartsWith(somespan, stackalloc[] {somevalue});

@Sergio0694
Copy link
Contributor

@MihaZupan should we also consider StartsWith/EndsWith overloads taking a SearchValues<T> parameter?

@MihaZupan
Copy link
Member Author

Do you mean something like a StartsWithAny?

static bool StartsWithAny<T>(ReadOnlySpan<T> span, SearchValues<T> values) =>
    span.Length > 0 && values.Contains(span[0]);

Things like span[0] is 'a' or 'b' or 'c' do come up sometimes, but I don't know how commonly compared to a single value. Wanting to do that with more than a few values such that you'd want a SearchValues feels niche.

Something like a bool StartsWithAny(ReadOnlySpan<char> span, SearchValues<string> values) could have an optimized implementation, but I'm not sure of the use cases.

Or did I misunderstand your question?

@Sergio0694
Copy link
Contributor

Yup, no that was my question indeed — I can see how it might probably be niche, plus the fact you can just roll your own version still leveraging SearchValues<T> by just doing values.Contains(span[0]) seems sufficient if anyone needs it I guess. As in, people could just add an extension like that using the code you showed, and it'd just as fast as a built-in API 🙂

Thanks!

@SimonCropp
Copy link
Contributor

any chance of this getting into v9?

@Sergio0694
Copy link
Contributor

Wondering if it could make more sense to just add params to those span parameters once C# 13 supports it? 🤔

@MihaZupan
Copy link
Member Author

@dotnet/area-system-memory should this one be marked as ready for review?

@tannergooding
Copy link
Member

We do provide similar overloads on string and it is quite convenient overall, so I'd lean towards marking this ready-for-review.

We could make some overloads params ReadOnlySpan<T> value, but not everywhere due to trailing parameters like StringComparison.

CC. @stephentoub, @GrabYourPitchforks for additional input.

@stephentoub
Copy link
Member

so I'd lean towards marking this ready-for-review

Seems fine.

Assuming the first-class span support lands, we wouldn't need the Span-based overloads, just the ReadOnlySpan-based overloads.

We could make some overloads params ReadOnlySpan value

I'm not following. All of the proposed overloads just take a T value... what would be params? The proposal would be to instead have them take a params ReadOnlySpan such that it becomes e.g. StartsWithAny?

@tannergooding
Copy link
Member

The proposal would be to instead have them take a params ReadOnlySpan such that it becomes e.g. StartsWithAny?

Right, sorry, that's more what I meant. Was mostly just thinking now that we have params ROSpan<T> there are more ways to handle cases like this in a friendly manner.

Having an explicit overload that just takes 1 value and does the least amount of checks/overhead is still goodness I think, though.

@tannergooding tannergooding added api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation untriaged New issue has not been triaged by the area owner labels Apr 3, 2024
@MihaZupan MihaZupan added this to the 9.0.0 milestone Apr 3, 2024
@GrabYourPitchforks
Copy link
Member

Having an explicit overload that just takes 1 value and does the least amount of checks/overhead is still goodness I think, though.

Agreed. This seems like the most common use case.

@bartonjs
Copy link
Member

bartonjs commented Jun 13, 2024

Video

Looks good as proposed.

There was a question of whether we're missing something by not having StartsWith(ROS<char>, char, StringComparison)/EndsWith, e.g. the German scharfes-S compared with "ss", but we can add that later if there's a demonstrated need.

namespace System;

public static partial class MemoryExtensions
{
    public static bool StartsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool StartsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.ReadOnlySpan<T> span, T value) where T : System.IEquatable<T>?;
    public static bool EndsWith<T>(this System.Span<T> span, T value) where T : System.IEquatable<T>?;
}

@bartonjs bartonjs 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 Jun 13, 2024
@stephentoub
Copy link
Member

Do we need the Span-based overloads? With the C# 13 features landing around spans, @333fred, we shouldn't need those for anything, right? Or will they still be needed to get a good extension method experience?

@333fred
Copy link
Member

333fred commented Jun 13, 2024

We shouldn't need them, the first-class spans proposal is specifically targeted at scenarios like this. /cc @jjonescz.

@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Jun 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jul 17, 2024
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 in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants