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 support to ReadOnlyMemory<char> #24064

Closed
stephentoub opened this issue Nov 6, 2017 · 7 comments
Closed

Add String support to ReadOnlyMemory<char> #24064

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

Comments

@stephentoub
Copy link
Member

Today ReadOnlySpan<char> can reference a System.String (via AsReadOnlySpan()), but ReadOnlyMemory<char> can't. We should fix that so that ReadOnlyMemory<char> can be used as a way to slice strings where the slices can live on the heap.

To me that means:

  1. Allowing ReadOnlyMemory<T> to wrap a string in addition to wrapping a T[] and an OwnedMemory<T>. This will mean an extra type check on operations like ReadOnlyMemory<T>.Span, but it can be guarded behind a typeof(T) == typeof(char) check such that the additional branch and few additional instructions will only impact T==char, and string will end up being a very common case for T==char, making it worth it.
  2. Adding an AsReadOnlyMemory() extension method forstring just as there's an AsReadOnlySpan() extension.
  3. Adding a TryGetString method on MemoryMarshal or some similar place that enables getting the string/offset/count out of a ReadOnlyMemory<char> if it's wrapping one.
  4. Whatever string operation methods we want exposed added as extension methods for ReadOnlyMemory<char>, related to https://github.com/dotnet/corefx/issues/20378 and https://github.com/dotnet/corefx/issues/21395

cc: @KrzysztofCwalina, @ahsonkhan, @terrajobst, @jkotas

Replaces https://github.com/dotnet/corefx/issues/20378?

@terrajobst
Copy link
Member

Adding a few more folks that asked for StringSegment before

@davidfowl @davkean

@stephentoub
Copy link
Member Author

stephentoub commented Nov 7, 2017

For API review for items 2 and 3 above:

namespace System
{
    public static class SpanExtensions  // Existing class containing AsReadOnlySpan(this string)... use it, something else, or rename it?
    {
        public static ReadOnlyMemory<char> AsReadOnlyMemory(this string text);
        ...
    }
}

namespace System.Runtime.InteropServices
{
    public static class MemoryMarshal // class added in https://github.com/dotnet/corefx/issues/23879#issuecomment-340861229
    {
        public static bool TryGetString(this ReadOnlyMemory<char> readOnlyMemory, out string value, out int offset, out int count);
        ...
    }
}

Alternative forms of TryGetString to consider instead?

public static string TryGetString(this ReadOnlyMemory<char> readOnlyMemory, out int offset, out int count);
public static (String: string, Offset: int, Count: int) TryGetString(this ReadOnlyMemory<char> readOnlyMemory);

Adding these will be dependent on verifying as part of the work that the extra check for string does not unduly impact other uses of ReadOnlyMemory<char>.

@Joe4evr
Copy link
Contributor

Joe4evr commented Nov 7, 2017

Not a fan of those alternative suggestions for TryGetString(); I think every method whose name starts with Try should return a bool.

@stephentoub
Copy link
Member Author

I think every method whose name starts with Try should return a bool.

Those alternatives were more about the shape; the name could be different.

@stephentoub stephentoub self-assigned this Nov 7, 2017
@terrajobst
Copy link
Member

terrajobst commented Nov 7, 2017

Video

Based on the discussion with @stephentoub we concluded that we should merge the two types:

namespace System
{
    public static class MemoryExtensions
    {
        // Existing class containing AsReadOnlySpan(this string)
        public static ReadOnlyMemory<char> AsReadOnlyMemory(this string text);

        // We already have ReadOnlyMemory<T>.TryGetArray(out ArraySegment<T> segment)
        public static bool TryGetString(this ReadOnlyMemory<char> readOnlyMemory, out string value, out int offset, out int count);
    }
}

@karelz
Copy link
Member

karelz commented Nov 8, 2017

FYI: The API review discussion was recorded - see https://youtu.be/HnKmLJcEM74?t=1163 (28 min duration)

@stephentoub
Copy link
Member Author

Fixed by dotnet/corefx#25120

@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 19, 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.Memory
Projects
None yet
Development

No branches or pull requests

5 participants