Consider adding HalfToInt16Bits and Int16BitsToHalf to BitConverter

[pgovind]

#37630 adds HalfToInt16Bits and Int16BitsToHalf as internal methods to BitConverter. We should consider exposing them as public.

Proposed API

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        internal static unsafe short HalfToInt16Bits(Half value);

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        internal static unsafe Half Int16BitsToHalf(short value);

cc: @tannergooding

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

[tannergooding]

This isn't critical for .NET 5 so marking as future. This just provides parity with the equivalents exposed for Single/Double.

[eiriktsarpalis]

For CBOR requirements we'd probably also need to add the following methods to the BinaryPrimitives class:

public static class BinaryPrimitives
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static Half ReadHalfLittleEndian(ReadOnlySpan<byte> source);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static Half ReadHalfBigEndian(ReadOnlySpan<byte> source);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool TryReadHalfLittleEndian(ReadOnlySpan<byte> source, out Half);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool TryReadHalfBigEndian(ReadOnlySpan<byte> source, out Half);

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static void WriteHalfLittleEndian(Span<byte> destination, Half value);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static void WriteHalfBigEndian(Span<byte> destination, Half value);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool TryWriteHalfLittleEndian(Span<byte> destination, Half value);
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static bool TryWriteHalfBigEndian(Span<byte> destination, Half value);
}

I feel that we could include these to the existing API proposal. If not I can file a separate issue.

[tannergooding]

Given its a separate class, it would likely be good to have in a separate API proposal.

The CBOR implementation wouldn't be blocked on these APIs being public, correct? That is, it can be implemented as an internal API until API review can happen, correct?

[eiriktsarpalis]

That is, it can be implemented as an internal API until API review can happen, correct?

That's correct

[terrajobst]

Video

Looks good

namespace System
{
    public sealed class BitConverter
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static short HalfToInt16Bits(Half value);

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        public static Half Int16BitsToHalf(short value);
    }
}

[huoyaoyuan]

For consistency, there are 4 more overloads for Half can be added:

public static Half ToHalf(byte[] value, int startIndex);
public static Half ToHalf(ReadOnlySpan<byte> value);
public static byte[] GetBytes(Half value);
public static bool TryWriteBytes(Span<byte> destination, Half value);

Do I need to create a new API proposal because this one is already approved?