Runtime: API proposal: BinaryPrimitives.ToLittleEndian / FromLittleEndian

[GrabYourPitchforks]

There are some scenarios (see https://github.com/dotnet/coreclr/pull/21948#discussion_r273272627) where we want to take a value in machine-endian order and convert it to little-endian. This is the opposite of normal APIs like htonl and ntohl, which convert between machine-endian and big-endian.

__Proposed API additions__

namespace System.Buffers.Binary
{
    // NEW methods added to EXISTING type
    public static class BinaryPrimitives
    {
        // convert from machine-endian to little-endian
        public static uint ToLittleEndian(uint value);

        // convert from little-endian to machine-endian
        public static uint FromLittleEndian(uint value);
    }
}

There would be overloads matching the existing overloads on ReverseEndianness: byte, sbyte, short, ushort, int, uint, long, ulong.

Open question: Do we also have ToBigEndian / FromBigEndian for parity and discoverability? Those APIs already exist on IPAddress.HostToNetworkOrder and IPAddress.NetworkToHostOrder. But those APIs require your application to pull in the package __System.Net.Primitives__, whereas these would all be contained within a single type.

[tannergooding]

Do we also have ToBigEndian / FromBigEndian for parity and discoverability

My vote is yes.

[HumanEquivalentUnit]

Open question: Do we also have ToBigEndian / FromBigEndian for parity and discoverability? Those APIs already exist on IPAddress.HostToNetworkOrder and IPAddress.NetworkToHostOrder.

NB. those don't work properly, they are broken; see https://github.com/dotnet/corefx/issues/36030#issuecomment-472667952 - they are using 64-bit signed ints to work on 32-bit IPv4 addresses, but when they swap the byte order around they reverse all 64 bits instead of just the low 32-bits.

[GrabYourPitchforks]

You mean if somebody has code that looks like this?

uint input = GetSomeUInt32();
var output = IPAddress.HostToNetworkOrder(input); // whoops, actually called the Int64 overload!

Yeah, that's a pit of failure, especially since var masks what the problem is. I imagine we could add all missing overloads in one shot.

[HumanEquivalentUnit]

@GrabYourPitchforks That isn't quite what I mean, and apologies if I'm dragging this issue off topic, but:

An IPv4 address is 4 bytes; store it in an Int64 like 0x00000000424344FF and call IPAddress.HostToNetworkOrder() you should get "(a reversed IP address) stored in an int64 0x00000000FF444342" but you actually get "a reversed int64 (with the IP address moved or corrupted) 0xFF44434200000000" which is mixing up the IP value and the implementation detail of how it's stored.

Those APIs should communicate "IPAddress to/from BigEndian", not "binary primitive" to/from BigEndian.

[GrabYourPitchforks]

I think the confusion stems from the fact that those methods on IPAddress are actually htonll and ntohll equivalents (and are documented as such), but the fact that they exist on the IPAddress class specifically makes it seem like they're intended to operate on IP addresses instead of arbitrary integers. It's unfortunate but I don't think we'd change it at this point.

[scottdorman]

Do we also have ToBigEndian / FromBigEndian for parity and discoverability?

Yes.

Those APIs already exist on IPAddress.HostToNetworkOrder and IPAddress.NetworkToHostOrder. But those APIs require your application to pull in the package System.Net.Primitives, whereas these would all be contained within a single type.

Since BinaryPrimitives is intended to be just as its name implies, a lower level primitives helper, it seems like the APIs on IPAddress should really be updated to call these new methods instead. (So essentially the relevant implementaton details in IPAddress.HostToNetworkOrder and IPAddress.NetworkToHostOrder move to the corresponding methods in BinaryPrimitives. That provides a single (assuming there aren't any other places in the framework doing this type of work) code path for it that any higher-level/upstream callers can use without recreating it.

[terrajobst]

Video

We need a more compelling reason. Sorry Levi :-)

[alfredmyers]

It's unfortunate it's being rejected. Although I don't remember the specifics, I clearly remember months ago finding it odd that this functionality was buried in IPAddress instead of being made available in a more generalized way.

[GrabYourPitchforks]

The reasoning that we settled on in API review is that we should be encouraging developers to use the APIs that read from / write to Span<byte>, and those APIs can handle endianness conversions appropriately.