Missing 64-bit ByteOrder conversions

General JUCE discussion
1

Where are these?

    /** Turns 8 bytes into a little-endian integer. */
    static uint64 littleEndianInt64 (const void* bytes);

    /** Turns 8 bytes into a big-endian integer. */
    static uint64 bigEndianInt64 (const void* bytes);
2

I added this to ByteOrder. I believe it makes expressions more clear, like:

  int16 networkInteger = ByteOrder::toNetwork (myInteger);

Here’s what I added

    template <typename IntegralType, std::size_t size = sizeof (IntegralType)>
    struct Swapper;

    template <typename IntegralType>
    struct Swapper <IntegralType, 2>
    {
        IntegralType operator() (IntegralType value)
        { return static_cast <IntegralType> (swapIfLittleEndian (uint16(value))); };
    };

    template <typename IntegralType>
    struct Swapper <IntegralType, 4>
    {
        IntegralType operator() (IntegralType value)
        { return static_cast <IntegralType> (swapIfLittleEndian (uint32(value))); };
    };

    template <typename IntegralType>
    struct Swapper <IntegralType, 8>
    {
        IntegralType operator() (IntegralType value)
        { return static_cast <IntegralType> (swapIfLittleEndian (uint64(value))); };
    };

    /** Converts from machine byte order to network byte order. */
    template <typename IntegralType>
    static IntegralType toNetwork (IntegralType value)
    { return Swapper <IntegralType> () (value); }

    /** Converts from network byte order to machine byte order. */
    template <typename IntegralType>
    static IntegralType fromNetwork (IntegralType value)
    { return Swapper <IntegralType> () (value); }
3

Thanks, I’ll add the 64-bit stuff.

The whole “to/from network” thing has always irritated me, as it just all seems so horribly designed to me… Effectively what the function should be called is “flip the byte order of this integer if the CPU’s endianness is different to the endianness used in network packets”, which is confusing enough. But it also feels wrong to me that you should ever need such a function… If something provides you with some network data in a structure, then surely it has the responsibility to make sure that the structure members aren’t garbage…? If it was returning the data as a pile of raw bytes, then ok, it would have no responsibility to fix it, but in that case you’d want some to/from network functions which convert between a sequence of network bytes to an integer, rather than being functions to flip the order of an integer… Anyways… end of rant.

4

LOL Jules. You of all people should care the most about writing clear code thats easy to read. “fromNetworkByteOrder()” is pretty clear. Here’s an example, its a piece of code that analyzes bytes from a client handshake to determine if it initiating an SSLv2 connection:

template <typename ConstBufferSequence>
void analyze (ConstBufferSequence const& buffer)
{
    Input <bytesNeeded> in (buffer);

    if (! in.peek (1))
        return;

    // First byte must have the high bit set
    //
    if ((in [0] & 0x80) != 0x80)
        return fail ();

    // The remaining bits contain the
    // length of the following data in bytes.
    //
    uint16 raw_length;
    if (! in.read (&raw_length))
        return;

    // sizeof (msg_type +
    //         Version (ProtcolVersion?) +
    //         cipher_spec_length +
    //         session_id_length +
    //         challenge_length)
    //
    // Should be 9 or greater.
    //
    uint16 const msg_length = fromNetworkByteOrder (raw_length) & 0x7fff;
    if (msg_length < 9)
        return fail ();

    if (! in.read (1))
        return;

    // The msg_type must be 0x01 for a version 2 ClientHello
    //
    if (in [0] != 0x01)
        return fail ();

    conclude ();
}

It’s all template based so using a MemoryInputStream is not really an option here, because ConstBufferSequence is a scatter/gather type of I/O. I rolled my own simple template class call Input which operates on objects that meet the requirements of a ConstBufferSequence

The call to fromNetworkByteOrder is VERY clear.

You would have written this:

uint16 const msg_length = swapIfLittleEndian (raw_length) & 0x7fff;

This violates one of the principles from the book The Art of Readable Code

A function should not describe exactly what it does, it should inform the user of the semantics. fromNetworkByteOrder is self-documenting. swapIfLittleEndian is not.

5

swapIfCPUEndiannessDiffersFromNetworkOrder (int16) is the shortest name I can think of which really describes what it does.

Or getInt16FromNetworkOrder (const void* rawBytesInNetworkOrder).

The thing I dislike is the fact that it takes an int as its parameter - that means that at some point in your code you have a variable which is an int16, but whose contents are junk unless you remember to flip it. Maybe ideally you’d want:

[code]struct NetworkInt16
{
int16 getValue() const;
void setValue (int16);

char rawDataInNetworkOrder[2];

};[/code]

6

I also disagree with how the functions each have a different name and they are stuck in an empty class (although the class doesn’t really bother me all that much). For example:

    static uint16 littleEndianShort (const void* bytes);
    static uint32 littleEndianInt (const void* bytes);
    static uint64 littleEndianInt64 (const void* bytes);
    static uint16 bigEndianShort (const void* bytes);
    static uint32 bigEndianInt (const void* bytes);
    static uint64 bigEndianInt64 (const void* bytes);

This cannot be extended to user-defined types. TLS protcol has a uint24 field. LOL! Yes you heard that right, a 3 byte integer. I had to roll my own class uint24.

But how to extend your byte swapping functions? There’s no way to do that because swap() is a plain old function relying on regular operator overloading. You’d have to edit juce_ByteOrder.h and add your type which is of course ridiculous.

I added a template class [url=https://gist.github.com/anonymous/f3f83c744f24be486386#file-beast_byteswap-h-L35]detail::SwapBytes[/url] which can be specialized for user defined types like uint24. Or as often comes up in peer to peer apps that use crypto, uint256 or uint512.

As you can see at the bottom there’s the swapBytes function which uses template argument deduction to choose the right specialization so the user doesn’t have to worry about any of that complexity.

7

I think I used an empty class rather than a namespace because there was some kind of declaration order issue, but can’t remember exactly what it was now. This is pretty old code.

8

Come to think of it, I prefer the class.

It pisses me off that boost::asio::ip is a namespace, because it would be nice to use as a template parameter.

9

I think you’re starting to see why I forked it so hard…lol