dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.67k stars 4.58k forks source link

API proposal: safe extension methods for converting Span<T> to Span<U> for primitive integral types #31305

Open GrabYourPitchforks opened 4 years ago

GrabYourPitchforks commented 4 years ago

This proposal is to make safe, simple APIs for things that would've been allowed using array variance. For example, it's fine to convert between byte[] and sbyte[]. The APIs proposed here are safe, will always succeed, and are zero-cost (reinterpret_cast<T> equivalent). The general idea is to avoid requiring developers to call unsafe MemoryMarshal APIs in order to achieve these conversions.

namespace System
{
    // new methods on existing static type
    public static class MemoryExtensions
    {
        public static ReadOnlySpan<byte> AsBytes(this ReadOnlySpan<sbyte> span);
        public static ReadOnlySpan<sbyte> AsSBytes(this ReadOnlySpan<byte> span);
        public static ReadOnlySpan<short> AsInt16(this ReadOnlySpan<ushort> span);
        public static ReadOnlySpan<ushort> AsUInt16(this ReadOnlySpan<short> span);
        public static ReadOnlySpan<int> AsInt32(this ReadOnlySpan<uint> span);
        public static ReadOnlySpan<uint> AsUInt32(this ReadOnlySpan<int> span);
        public static ReadOnlySpan<long> AsInt64(this ReadOnlySpan<ulong> span);
        public static ReadOnlySpan<ulong> AsUInt64(this ReadOnlySpan<long> span);

        /* also Span-based overloads */
        /* also nint / nuint-based overloads if we want those */
    }
}

In particular, there are no APIs that go from arbitrary T to these primitive integral types, since that would run afoul of https://github.com/dotnet/corefx/issues/27094. Additionally, this proposal does not include APIs which would cause the resulting span to have a different length, as that would result in endianness issues and would require processing beyond a simple reinterpret_cast<T>. The MemoryMarshal.Cast and MemoryMarshal.AsBytes APIs remain available to callers who really need that scenario.

jkotas commented 4 years ago

Are these conversions needed often enough to deserve an API? I would not feel bad to recommend using MemoryMarshal.Cast for the few rare cases where these conversions are needed.

NickStrupat commented 4 years ago

@jkotas I think the concern with MemoryMarshal.Cast is that the safety/portability is unclear. I have seen the comments added to the documentation, but they don't inspire confidence as they are now. I imagine other, including @GrabYourPitchforks, feel the same way.

Another way of looking at it is that it just "feels sketchy" using an assumed-safe part of MemoryMarshal when you know other parts are unsafe depending on the context.

I believe the best developer ergonomics arise from the most obvious possible separation of safe and unsafe operations. The usual abstraction for separation of APIs is between classes.

jkotas commented 4 years ago

I understand the theory about why these APIs may be better.

My concern is that the scenario that these APIs cover is super rare. For example, I do not see a single place in CoreFX where these APIs can be used. If the low-level API like this cannot be used in CoreFX itself. it suggests that the API is useless.

NickStrupat commented 4 years ago

Low/zero-allocation binary serialization and deserialization is my main use case for this kind of stuff.

GrabYourPitchforks commented 4 years ago

Low/zero-allocation binary serialization and deserialization is my main use case for this kind of stuff.

There's no arbitrary conversion to ROS<byte> as part of this proposal, so I don't think it helps the binary serialization scenario much. It would be mainly to allow some forms of variance that right now require calls to the unsafe MemoryMarshal.Cast API. For instance: