decimal forgotten by Span<T> APIs

[gbieging]

Edit by @GrabYourPitchforks on 29 Jan 20: API proposal is in a comment at https://github.com/dotnet/corefx/issues/35877#issuecomment-471004904

I was recently toying with making a serialization library and found out that there are no Span<T> APIs for serializing and deserializing decimals.

I searched for the issues related to the Span<T> APIs and found #18969 and #18970 for decimal, but when they where closed for tracking in #19444 decimal was forgotten.

I know 3.0 is near but it would be nice to have it in 2.x too.

[stephentoub]

What APIs are you referring to?

decimal has Parse/TryParse overloads that take ReadOnlySpan<T>, and a TryFormat that takes Span<T>.

[gbieging]

For binary serialization. There are no methods in BitConverter to convert from/to decimal and Decimal.GetBits does not take Span<T>.

[stephentoub]

I see. @tannergooding, @pentp, what (if anything) would make sense to do here?

[tannergooding]

I think providing:

public readonly partial struct Decimal
{
    public Decimal(ReadOnlySpan<int> bits);
    public static bool TryGetBits(decimal d, Span<int> bits);
}

Makes sense. Decimal is currently the only non-binary floating-point type we provide and we should allow it to be serialized/deserialized in a non-allocating manner.

[Wraith2]

I found this a couple of weeks ago as well. I think it wasn't implemented through lack of clear demand for it having discussed it on corefx gitter. There could be a decimal.TryGetBits(Span<int>) mirroring the existing int[] GetBits() allowing allocation free retrieval of the decimal contents for serialization. I don't know whether there would also be a desire for a byte version.

[pentp]

Decimal has an internal method ToDecimal(ReadOnlySpan<byte> span) used by BinaryReader and an internal GetBytes for BinaryWriter (but this one is suboptimal), these could be extended to a public contract.
One option would be to provide the public methods in BinaryPrimitives, but I noticed that float/double don't have such methods also.

[tannergooding]

but I noticed that float/double don't have such methods also.

That is actually tracked by https://github.com/dotnet/corefx/issues/35791. We should consider including decimal here as well.

[stephentoub]

One option would be to provide the public methods in BinaryPrimitives, but I noticed that float/double don't have such methods also.

https://github.com/dotnet/corefx/issues/35791

[pentp]

That looks like the right place! Should add decimal also to that issue and fix them together. BinaryPrimitives is better also because it explicitly calls out the endianess (BinaryReader/BinaryWriter are implicitly little-endian).

[ahsonkhan]

So, is the plan to only provide new BinaryPrimitive APIs (tracked by the other issue) OR do we also want to provide the span-based ctor on Decimal (as @tannergooding suggested)? If the former, is this issue redundant? If the latter, can we update OP to point to the actual API proposal here. In any case, updating the area.

[markrendle]

It does seem wrong that there is no Span<int> overload for decimal.GetBits, or a decimal.TryGetBits. Sure, it probably doesn't get used often, but I'd imagine that when it does (as in the use case I've just run into) it's at the kind of point in a codebase where performance is critical and the overhead of GC allocating an int[4] is unwanted, where one could Span<int> bits = stackalloc int[4] so easily.

My use case, for informational purposes, is to safely write decimal values to Protobuf, which only natively supports float and double. It's easy to work around this by creating a Decimal message type, thus:

message Decimal {
  uint32 lo = 1;
  uint32 mid = 2;
  uint32 hi = 3;
  bool isNegative = 4;
  uint32 scale = 5;
}

In code the generated type can be extended with implicit operators to convert to and from System.Decimal, and it would be nice if that could be done without allocating an array on the GC heap.

[mikernet]

In the meantime, if you want something that works now:

private readonly struct DecimalData
{
    private const int SignMask = unchecked((int)0x80000000);
    private const int ScaleMask = 0x00FF0000;
    private const int ScaleShift = 16;

    public readonly int Flags;
    public readonly int Hi;
    public readonly int Lo;
    public readonly int Mid;

    public DecimalData(int flags, int hi, int lo, int mid)
    {
        if (!IsValid(flags))
            throw new InvalidDataException("Invalid decimal flag data.");

        Flags = flags;
        Hi = hi;
        Lo = lo;
        Mid = mid;
    }

    private static bool IsValid(int flags) => (flags & ~(SignMask | ScaleMask)) == 0 && ((uint)(flags & ScaleMask) <= (28 << ScaleShift));
}

public unsafe decimal ReadDecimal()
{
    var data = new DecimalData(ReadInt32(), ReadInt32(), ReadInt32(), ReadInt32());
    return *(decimal*)&data;
}

public unsafe void WriteDecimal(decimal value)
{
    DecimalData data = *(DecimalData*)&value;

    WriteInt32(data.Flags);
    WriteInt32(data.Hi);
    WriteInt32(data.Lo);
    WriteInt32(data.Mid);
}

Just change ReadInt32 / WriteInt32 to read/write to your span in whatever manner you deem appropriate.

[GrabYourPitchforks]

Is @tannergooding's API proposal at https://github.com/dotnet/corefx/issues/35877#issuecomment-471004904 the thing we want to bring through review?

[Wraith2]

Is there anything more needed than the two methods @tannergooding proposed before this can be api reviewed?

[tannergooding]

Sorry for the delay, I had missed the last reply to this thread.

Ideally the OP (@gbieging) would update the top post with the API intended to be reviewed.

The API review process is here: https://github.com/dotnet/runtime/blob/master/docs/project/api-review-process.md and we generally prefer the proposal to be in a format similar to the good example that is given (with a rationale and proposed API section; if nothing else). It just helps speed along the review process and make everything clear.

Otherwise, someone (generally the area owner) has to tell the meeting driver (generally @terrajobst) to jump down to this other comment that actually contains the proposed API, summarize the thinking behind it, and call out any notable "open questions" or other details called out by the overall thread.

[GrabYourPitchforks]

I updated the top comment since Tanner already did the heavy lifting of making the API proposal. ;)

[ahsonkhan]

Is there a need for the try-pattern API here? If so, maybe include one that throws:

public static bool TryGetBits(decimal d, Span<int> bits);
public static void GetBits(decimal d, Span<int> bits);

// Existing:
public static int[] GetBits(Decimal d);

[Wraith2]

I think the idea is to provide some symmetry with the BitConverter.TryGet methods which don't throw. If you're dealing with something like Pipelines and can't write all the bytes to the existing buffer you don't want to be using exceptions to signal failure you just try get a false then ask for a larger buffer and retry.

[terrajobst]

• Based on @tannergooding's comment
• TryGetBits should follow the span pattern
• Given the size is known, we should also have a throwing GetBits()

namespace System
{
    public readonly partial struct Decimal
    {
        public Decimal(ReadOnlySpan<int> bits);
        public static bool TryGetBits(decimal d, Span<int> destination, out int valuesWritten);
        public static int GetBits(decimal d, Span<int> destination);
    }
}

[ArjunVachhani]

@stephentoub how to use new Span APIs with Net Standard 2.1 library? will it work with only .net 5?

[stephentoub]

They're new in .NET 5, and thus aren't available via .NET Standard 2.1.