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

Add String Span-based method for writing into String's buffer #22819

Closed
4 tasks done
stephentoub opened this issue Jul 18, 2017 · 7 comments
Closed
4 tasks done

Add String Span-based method for writing into String's buffer #22819

stephentoub opened this issue Jul 18, 2017 · 7 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime
Milestone

Comments

@stephentoub
Copy link
Member

Separated out of https://github.com/dotnet/corefx/issues/21281 for tracking purposes.

  • Implement in System.Private.CoreLib in coreclr
  • Implement in System.Private.CoreLib in corert (if string isn't yet "shared" between coreclr and corert)
  • Expose from System.Runtime contract in corefx
  • Add tests to System.Runtime tests in corefx
namespace System
{
    public delegate void StringCreateAction<TState>(TState state, Span<char> destination);

    public class String
    {
        public static string Create<TState>(int length, TState state, StringCreateAction action);}
}

Mostly approved, but open issue around the delegate, its name, what namespace it lives in (or nested type within String), etc. A custom delegate is needed as span can't be used as a generic type argument with one of the built-in action types.

@karelz
Copy link
Member

karelz commented Sep 5, 2017

Approved in this shape:

namespace System.Buffers
{
    public delegate void SpanAction<T, in TArg>(Span<T> span, TArg arg);
    public delegate void ReadOnlySpanAction<T, in TArg>(ReadOnlySpan<T> span, TArg arg);
}

namespace System
{
    public class String
    {
        public static string Create<TState>(int length, TState state, SpanAction<char, TState> action);
    }
}

Discussed points:

  • We will use System.Buffers namespace as thre will be more actions like this, we do not want to pollute System namespace.
  • SpanAction is better than ActionOverSpan (lexicographic ordering)
  • Order of args and naming is similar to Action

@karelz
Copy link
Member

karelz commented Sep 5, 2017

Added ReadOnlySpanAction and moved to System.Buffers based on review of #22472

@ektrah
Copy link
Member

ektrah commented Sep 7, 2017

Span<T> and ReadOnlySpan<T> cannot be used as a type argument, so the above Create<TState> method cannot be used with spans as input/state. I can imagine that could be something fairly common to do. Could another overload be added?

public delegate void ReadOnlySpanSpanAction<TIn, TOut>(
    ReadOnlySpan<TIn> input, Span<TOut> output);

public class String
{
    public static string Create<TIn>(
        int length, ReadOnlySpan<TIn> state, ReadOnlySpanSpanAction<TIn, char> action);
}

Two examples from https://github.com/dotnet/corefx/issues/21281 that could benefit from this overload:

namespace System
{
    public class Convert
    {
        public static string ToBase64String(ReadOnlySpan<byte> bytes, Base64FormattingOptions options = Base64FormattingOptions.None);
    }
}

namespace System.Text
{
    public class Encoding
    {
        public virtual string GetString(ReadOnlySpan<byte> bytes);
    }
}

(Both are currently implemented with FastAllocateString.)

@stephentoub stephentoub self-assigned this Sep 8, 2017
@stephentoub
Copy link
Member Author

Could another overload be added?

I suggest we wait until there's a proven need for it. While you may be right and it could end up being common, most of the cases I've seen historically are not based on taking in string-like data as input, but rather some other kind of data that's then used to produce characters. Often these characters are stored into a heap-allocated char[], so this new overload avoids that allocation and then the copy from it into the string, or into a stack-allocated char*, in which case this new overload helps avoid the copy.

@justinvp
Copy link
Contributor

justinvp commented Sep 8, 2017

Is there a planned use for ReadOnlySpanAction or scenario in mind where it'd be used? If not, should we hold-off on adding it until there's a proven need?

@stephentoub
Copy link
Member Author

Is there a planned use for ReadOnlySpanAction or scenario in mind where it'd be used? If not, should we hold-off on adding it until there's a proven need?

@KrzysztofCwalina, I believe you asked for this? Or am I misremembering?

@stephentoub
Copy link
Member Author

Is there a planned use for ReadOnlySpanAction or scenario in mind where it'd be used?

I just remembered where:
https://github.com/dotnet/corefx/issues/21472

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 21, 2020
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
Projects
None yet
Development

No branches or pull requests

5 participants