API Proposal: SafeBuffer Span<T> methods

[vcsjones]

Proposal:

namespace System.Runtime.InteropServices
{
    public abstract unsafe class SafeBuffer
    {
        public void ReadSpan<T>(ulong byteOffset, Span<T> buffer) where T : struct => throw null;
        public void WriteSpan<T>(ulong byteOffset, ReadOnlySpan<T> data) where T : struct => throw null;
    }
}

The purpose of this is to be a Span-based equivalent of ReadArray. The motivation for this is for a later API proposal to add the same methods to UnmanagedMemoryAccessor which will work for memory mapped views.

This would make it possible to work efficiently and safely with stackalloc'ed data.

[karelz]

cc @GrabYourPitchforks @stephentoub

[stephentoub]

Seems reasonable to me. These are just direct span-based equivalents of the existing array methods:

public void ReadArray<T>(ulong byteOffset, T[] array, int index, int count) where T : struct;
public void WriteArray<T>(ulong byteOffset, T[] array, int index, int count) where T : struct;

[jeffschwMSFT]

cc @AaronRobinsonMSFT

[AaronRobinsonMSFT]

Not necessarily against these, but if I recall the SafeBuffer class is marked as obsolete. I don't see it in code, but it is on docs.microsoft.com - Seems kind of silly to be extending an obsolete API?

Other than perhaps placement, this seems like a good idea.

[stephentoub]

obsolete

Interesting. What does it mean for SafeBuffer to be obsolete but SafeMemoryMappedViewHandle derived from it to not be?

[AaronRobinsonMSFT]

@stephentoub That is a great question. This has been marked this way for a long time if I recall - well before I came on the scene. Can you think of anyone who might know why this class was officially marked as obsolete?

[AaronRobinsonMSFT]

Looks like this is happening because of something from netstandard 1.2 😮 Perhaps a mistake?

https://github.com/dotnet/dotnet-api-docs/blob/233109a79714842b6ce02e919ad1cb078267f434/xml/System.Runtime.InteropServices/SafeBuffer.xml#L43-L45

[jkotas]

People were pretty liberal in early days of .NET Core with marking things obsolete. Most/all of it was rolled back. I am not able to find the history for this particular one, but here are a few related PRs:

dotnet/corefx#17748
dotnet/corefx#10541

[vcsjones]

I think it's possible to implement the proposal directly on SafeMemoryMappedViewHandle which is what I was going for anyway.

We have access to the handle and length via ByteLength.

Does it make more sense to do that or try and undeprecate SafeBuffer?

[jkotas]

SafeBuffer is not deprecated. The documentation needs to be fixed: dotnet/dotnet-api-docs#1184

[AaronRobinsonMSFT]

@vcsjones Now that the SafeBuffer discussion is out of the way 😄 I think this is a good suggestion. I am all for it.

[vcsjones]

Happy to do a PR for this when it gets through API review.

[ahsonkhan]

We didn't discuss the parameter names in the review. Should they both be called span to match the array overloads (or both be called buffer)? Why is one called buffer and the other data?

public void ReadSpan<T>(ulong byteOffset, Span<T> buffer) where T : struct => throw null;
public void WriteSpan<T>(ulong byteOffset, ReadOnlySpan<T> data) where T : struct => throw null;

[vcsjones]

Why is one called buffer and the other data?

Hm. Probably just a poor decision on my part when writing this up.

Thould they both be called span to match the array overloads (or both be called buffer)?

I think I avoided span for both because one is a ReadOnlySpan and calling it span seemed out of place. If that is an ill-placed thought then span works for me, as does buffer.

[GrabYourPitchforks]

Looks like this is unassigned. @vcsjones did you want to take this or should we mark it up for grabs?

[vcsjones]

@GrabYourPitchforks I'm happy to pick it up; wanted to know how we should resolve @ahsonkhan's feedback post API Review process.

[GrabYourPitchforks]

It's fine to call the parameter name span IMO. We do this for other APIs which accept ReadOnlySpan<T> as a parameter. Check https://apisof.net/catalog/System.MemoryExtensions.IndexOf%3CT%3E(ReadOnlySpan%3CT%3E,T) for one such example.

[ahsonkhan]

It's fine to call the parameter name span IMO. We do this for other APIs which accept ReadOnlySpan<T> as a parameter. Check https://apisof.net/catalog/System.MemoryExtensions.IndexOf%3CT%3E(ReadOnlySpan%3CT%3E,T) for one such example.

Granted, that's an extension method. I think either span or buffer is fine as long as we are consistent.

[AaronRobinsonMSFT]

@vcsjones Where did we land on this? Did I miss the PR?

[stephentoub]

This is marked as api-approved. Presumably what's next is just for someone to implement it 😄

[vcsjones]

Hm, I guess that can be me. I see I offered a little while ago but failed to follow through. Apologies.

[stephentoub]

No need to apologize :) Thanks for the offer.