Move Span.DangerousCreate to MemoryMarshal.Create

[KrzysztofCwalina]

current API:

    public struct Span<T> {
        public static Span<T> DangerousCreate(object obj, ref T objectData, int length);
    }
    public struct ReadOnlySpan<T> {
        public static ReadOnlySpan<T> DangerousCreate(object obj, ref T objectData, int length);
    }

proposed API:

    public static class MemoryMarshal {
        public static Span<T> CreateSpan(object obj, ref T objectData, int length);
        public static ReadOnlySpan<T> CreateReadOnlySpan(object obj, ref T objectData, int length);
    }

[tarekgh]

Just raising the question, do we need to have similar API for {ReadOnly}Memory? I mean

    public static class MemoryMarshal {
        public static Memory<T> CreateMemory(object obj, ref T objectData, int length);
        public static ReadOnlyMemory<T> CreateReadOnlyMemory(object obj, ref T objectData, int length);
    }

per the comment https://github.com/dotnet/corefx/issues/24296#issuecomment-341586055 on #23670 looks not preferred though.

[KrzysztofCwalina]

No, Memory is not capable of storing interior pointers/refs

[tarekgh]

Sorry I meant something like

public static class MemoryMarshal {
    public static Memory<T> CreateMemory(T[] array, int start, int length);
    public static ReadOnlyMemory<T> CreateReadOnlyMemory(T[] array, int start, int length);
}

This is just will not validate parameter (similar to DangerousCreate).

I don't think we need it, I am just want to be an explicit decision we don't need it.

[KrzysztofCwalina]

I think parameter validation might be an issue for Spans which are super optimized and the validation can add up. Memory APIs have enough meat in them that I think the validation overhead is not significant. But, if people complain and show such overhead, we could always add such APIs in the next release.

[jkotas]

I think we should kill the Create overload that takes first argument as object. People always use it incorrectly (e.g. by passing the first argument as null). Just run into another case: dotnet/corefx#26289 (comment) . I have not seen a correct use it of it.

Instead, we should have Span<T> MemoryMarshal.Create<T>(ref T data, int lenght) in the netcoreapp version (ie fast Span) of System.Memory only.

[jkotas]

Also, we may want to rename NonPortableCast to MemoryMarshal.Cast to get rid of another ugly name.

[ahsonkhan]

Also, we may want to rename NonPortableCast to MemoryMarshal.Cast to get rid of another ugly name.

Updating the proposal

Edit: Creating new issue instead

[ahsonkhan]

I think we should kill the Create overload that takes first argument as object.
Instead, we should have Span<T> MemoryMarshal.Create<T>(ref T data, int lenght) in the netcoreapp version (ie fast Span) of System.Memory only.

@KrzysztofCwalina, if you agree, please update the original proposal to remove the first argument (object obj).

[KrzysztofCwalina]

I agree that the name is ugly and that it is an eyesore on MemoryExtensions/Span. So I like the rename to "Cast". I am not totally convinced the method needs to go to MemoryMarshal. The method is super useful and people will be using it no netter where it is and where it lives, especially that it is very much portable and safe in 99% of the cases. The gymnastics we go through to deswade people from using it will hurt 99% of the users and I doubt will help the 1%; they will have to discover the problems the hard way and change thir code.

@terrajobst, @weshaggard

[jkotas]

@ahsonkhan Opened dotnet/corefx#26368 on the Cast, so we may rather discuss it there.

I agree that the Cast is useful, but you need to know what you are doing. I think there needs to be a small speed-bump to get to it to make it clear.

[jkotas]

For example, we had to take a pretty no-trivial servicing fix in .NET Core 2.0 for Windows ARM because the code played it loose with alignment: dotnet/corefx#25763 . Span.Cast encourages issues like that if you are not careful.

[ahsonkhan]

I agree that the name is ugly and that it is an eyesore on MemoryExtensions/Span.

I was referring to removing the object argument from the DangerousCreate methods in the proposed API and only have it be a netcoreapp specific API.