Consider adding BitConverter overloads that accept endian option

[stephentoub]

#22803 lists APIs we're planning to add that use Span<byte> instead of byte[]. These and the existing methods also use the current machine's endianness. We should separately consider overloads that accept the endianness to use, rather than always using that of the current machine, e.g.

public static bool TryWriteBytes(Span<byte> destination, long value, bool? littleEndian);
public static long ToInt64(ReadOnlySpan<byte> value, bool? littleEndian);
// null littleEndian means current machine's

or

public enum Endianness
{
    Current,
    Little,
    Big
}

public static bool TryWriteBytes(Span<byte> destination, long value, Endianness endianness);
public static long ToInt64(ReadOnlySpan<byte> value, Endianness endiannes);

cc: @bartonjs

[bartonjs]

I think the enum makes it clearer to read, but if we're consistent with parameters being either bigEndian or littleEndian then the bool? is accurately capable of describing it (unless we wanted to support Middle-endian, but that doesn't seem terribly likely. Right?)

I'd call the zero-value System or Native instead of Current to indicate that it's really the system-native byte order, and not that the "Current"ness can't be influenced by state.

Wikipedia also makes me wonder if we need the naming to indicate Byte endianness vs Bit endianness. (I do recall back in college and learning about Little Endian and being aghast at how it was really middle-endian, since I was thinking in terms of the bits... the "left-most" "in hardware" being 0x00000008).

Any particular middle-endian byte scheme would need specific descriptions, like PDP would probably be named something terrible like MiddleEndian_01000302 (or, y'know, PDP)).

So I think:

• I'd go with the enum. It reads better (and it allows flexibility we'll probably never use).
• I'd rename Current to Native.
• That (tangentially) I wish I could come up with a name for a parameter to tell Biginteger "this data was unsigned, please don't sniff the sign bit"

[mellinoe]

I strongly prefer the second version, although I know folks will not want to add a new "Endianness" enum into a core namespace. The first option feels like an abuse of the nullable type and looks a bit unpleasant to me. If we use the first option, we should flip the bool? parameter to isBingEndian such that false is a reasonable default (rather than true), given that most/all .NET implementations run on little-endian processors.

[bartonjs]

I guess an extra cost of the enum (aside from the typedef/Intellisense cost) is that we need to validation on it, or have a convention that says all unmapped values get treated as Native/Current. That said, I still think it enhances legibility.

[jnm2]

While I don't particularly agree about abuse of bool?, I do agree about readability.

[jkotas]

I guess an extra cost of the enum (aside from the typedef/Intellisense cost) is that we need to validation on it.

Agree. The enum makes the APIs slower than they need to be. What's wrong with having endianness to be part of the API name?

[Clockwork-Muse]

I appreciate the readability of the enum.

(That said, I'm of the opinion that methods shouldn't have boolean parameters, in favor of separate methods or perhaps an enum. Because otherwise "now the method does two things")

[PeterSmithRedmond]

I like having the tri-state enum. A good goal is to let people fall into the "pit of success" -- can we give Big Endian two different name? I'd love to have an enum like this:
{
0 = systemEndian,
1 = networkEndian
1 = bigEndian # same and networkEndia
2 = littleEndian
}

That way the two most common things people need to do -- convert system to network and network to system -- are super easy. If we can do this without using 'big' versus 'little', that IMHO is a gain.

I've never had to deal with "bit" endian-ness and IMHO adding special options for it (or even mentioning it in documentation) will only confuse our developers.

[Clockwork-Muse]

@PeterSmithRedmond - if we're adding "Network" as an option, does that mean SystemEndian should be HostEndian instead (to match with the NetworkToHost/HostToNetwork methods elsewhere)? Just curious here.

[stephentoub]

This isn't necessary with the BinaryPrimitives APIs being added in dotnet/corefx#24400.