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: String.{Try}CopyTo(Span<char>) #51061

Closed
stephentoub opened this issue Apr 11, 2021 · 2 comments · Fixed by #53246
Closed

API Proposal: String.{Try}CopyTo(Span<char>) #51061

stephentoub opened this issue Apr 11, 2021 · 2 comments · Fixed by #53246
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 11, 2021

Background and Motivation

When working with text (strings and spans), it's relatively common to need to copy a string to a span, which can be done with:

string.AsSpan().CopyTo(destination)

We can make this simpler and faster by adding such a CopyTo directly to string:

string.CopyTo(destination)

Same with TryCopyTo. For example, Number.Formatting.cs in corelib has this:

private static bool TryCopyTo(string source, Span<char> destination, out int charsWritten)
{
Debug.Assert(source != null);
if (source.AsSpan().TryCopyTo(destination))
{
charsWritten = source.Length;
return true;
}
charsWritten = 0;
return false;
}

by changing the source.AsSpan().TryCopyTo(destination) there to source.TryCopyTo(destination) (with an implementation of TryCopyTo on string that's the same 7-line implementation on span), we get this diff:
image

Proposed API

namespace System
{
    public sealed class String
    {
        ...
        public void CopyTo(int sourceIndex, char[] destination, int destinationIndex, int count);
+       public void CopyTo(Span<char> destination);
+       public bool TryCopyTo(Span<char> destination);
    }
}

Risks

string.AsSpan().{Try}CopyTo works with null input strings, because AsSpan() special-cases null to return an empty span. As a result, someone switching from the former to these new APIs could incur a null reference exception if they used a null string.

@stephentoub stephentoub added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Runtime labels Apr 11, 2021
@stephentoub stephentoub added this to the 6.0.0 milestone Apr 11, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 11, 2021
@GrabYourPitchforks
Copy link
Member

I think this category of API is generally useful. When I was experimenting with Utf8String I had an internal AsSpanSkipNullCheck method hanging off the type.

/// <summary>
/// Similar to <see cref="Utf8Extensions.AsSpan(Utf8String)"/>, but skips the null check on the input.
/// Throws a <see cref="NullReferenceException"/> if the input is null.
/// </summary>
[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal Utf8Span AsSpanSkipNullCheck()
{
return Utf8Span.UnsafeCreateWithoutValidation(this.AsBytesSkipNullCheck());
}

The general idea is that it was for scenarios where I really didn't expect the string instance to be null, and I was willing to take the NRE in order to avoid complicated branching in my hot paths.

@stephentoub stephentoub 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 11, 2021
@stephentoub stephentoub added the blocking Marks issues that we want to fast track in order to unblock other important work label May 22, 2021
@bartonjs
Copy link
Member

bartonjs commented May 25, 2021

Video

  • While our newer Span-writing methods return the number of elements written (when there's not a better purpose for the return value) this has symmetry with the current Span methods.
namespace System
{
    public sealed class String
    {
        ...
        public void CopyTo(int sourceIndex, char[] destination, int destinationIndex, int count);
+       public void CopyTo(Span<char> destination);
+       public bool TryCopyTo(Span<char> destination);
    }
}

@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 May 25, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 25, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 26, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 25, 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.Runtime blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants