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 Convert Span-based APIs #22848

Closed
3 tasks done
stephentoub opened this issue Jul 18, 2017 · 9 comments
Closed
3 tasks done

Add Convert Span-based APIs #22848

stephentoub opened this issue Jul 18, 2017 · 9 comments
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Runtime tenet-performance Performance related issue
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 ("shared" with corert)
  • Expose from System.Runtime.Extensions contract in corefx
  • Add tests to System.Runtime.Extensions tests in corefx
namespace System
{
    public class Convert
    {
        public static string ToBase64String(ReadOnlySpan<byte> bytes, Base64FormattingOptions options = Base64FormattingOptions.None);
        public static TransformationStatus ToBase64Chars(ReadOnlySpan<byte> bytes, Span<char> chars, out int bytesConsumed, out int charsWritten, Base64FormattingOptions options = Base64FormattingOptions.None);

        public static TransformationStatus FromBase64(string s, Span<byte> destination, out int charsConsumed, out int bytesWritten, Base46FormattingOptions options = ...);
        public static TransformationStatus FromBase64(ReadOnlySpan<char> s, Span<byte> destination, out int charsConsumed, out int bytesWritten, Base46FormattingOptions options = ...);}
}

Mostly approved, but needed to re-verify approach with TransformationStatus.

Depends on https://github.com/dotnet/corefx/issues/22412.

@stephentoub
Copy link
Member Author

We agreed in the API review to revert back to the original proposal:

namespace System
{
    public class Convert
    {
        public static string ToBase64String(ReadOnlySpan<byte> bytes, Base64FormattingOptions options = Base64FormattingOptions.None);
        public static bool TryToBase64Chars(ReadOnlySpan<byte> bytes, Span<char> chars, out int charsWritten, Base64FormattingOptions options = Base64FormattingOptions.None);
        public static bool TryFromBase64String(string s, Span<byte> bytes, out int bytesWritten);
        public static bool TryFromBase64Chars(ReadOnlySpan<char> chars, Span<byte> bytes, out int bytesWritten);}
}

@stephentoub stephentoub self-assigned this Oct 6, 2017
@karelz
Copy link
Member

karelz commented Oct 6, 2017

@stephentoub I thought we decided to postpone the decision for later, rather than making a decision ... https://github.com/dotnet/apireviews/blob/master/2017/10-03-GitHub%20issues.md

@stephentoub
Copy link
Member Author

I thought we decided to postpone the decision for later

We decided to postpone discussion of TransformationStatus. I just re-watched starting from 1:13:30... it seems like we agreed to go back to the original thing proposed without TransformationStatus, no?

@karelz
Copy link
Member

karelz commented Oct 6, 2017

@stephentoub it's quite possible, I didn't pay too much attention. If you think it is non-controversial, I am fine.

@stephentoub
Copy link
Member Author

@bartonjs, @KrzysztofCwalina, can you confirm my recollection?

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Oct 6, 2017

We postponed adding TransformationStatus to the core. The reason being that Convert APIs should stay simple for scenarios where a single buffer is being manipulated. TransformationStatus adds very significant complexity and is only useful when dis contiguous buffers need to be processed.

@bartonjs
Copy link
Member

bartonjs commented Oct 6, 2017

IIRC I was the original proponent of using TransformationStatus for Base64. In the Tuesday meeting I said I'd changed my mind and thought simple bool-Try was sufficient; but then I had to leave.

Assuming no one else disagreed after I left, seems good to me.

@hughbe
Copy link
Contributor

hughbe commented Mar 9, 2019

Has there been a discussion about adding Covert.To* overloads that take a span? I was thinking because I saw this code: https://github.com/dotnet/corefx/blob/master/src/System.ComponentModel.TypeConverter/src/System/ComponentModel/BaseNumberConverter.cs#L70

internal abstract object FromString(string value, int radix);
// ...
return FromString(text.Substring(2), 16);

We could get some perf benefits in situations where the radix is needed?

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@nikeee
Copy link

nikeee commented Apr 16, 2020

@hughbe I was thinking too about these methods of System.Convert in particular:

static class Convert {
    public static char ToBoolean(string value);
    public static char ToChar(string value);
    public static short ToInt16(string value);
    public static int ToInt32(string value);
    public static long ToInt64(string value);
    public static decimal ToDecimal(string value);
    public static double ToDouble(string value);
    // ...
}

Anything that prevents us from creating overloads that accept ReadOnlySpan<char>? Does it make sense to do so?

As fas as i can tall, the To* methods just pass the value to their respective *.Parse methods (ref). The *.Parse methods already accept a ReadOnlySpan<char>.

@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 tenet-performance Performance related issue
Projects
None yet
Development

No branches or pull requests

7 participants