Boost logo

Boost :

Subject: [boost] [Endian] Review
From: Tomas Puverle (Tomas.Puverle_at_[hidden])
Date: 2011-09-08 13:18:52

Hi Beman,

> - What is your evaluation of the design?

The design of the integer types seems reasonable to me, even though I would
prefer not to have the overloaded operators on them. I feel that having the
operators has the potential to break the boundary between program input and
the program's processing. What do you I mean by that? Typically, I try to
structure my programs as follows:

        1) Read input data
        2) Verify input invariants, construct internal representation
        3) Process internal representation
        4) Output result

The problem with having operators in the endian types is that they encourage
the user to continue using them past the point 2).

However, as I said, that's my personal preference and last year plenty of
people wanted to see the operators included. Perhaps they could be a policy?

endian<..., bool EnableOperators_ > ?

As for the rest of the library, I have the following comments:

- As many other people have mentioned, floating point support and user defined
types are a must.
- I am happy that the "conversion functions" have been added, Unfortunately,
I find them lacking in several departments:
        - (Note: I am going to try very hard not to get drawn into another naming
                discussion. That's pretty much what ended my efforts last year -
                50 people commenting, each proposing a different set of names :D )
        - While the endian types support unaligned data, the endian functions
                don't appear to do so. I don't see how the endian types could be
                implemented using them.
        - No support for user defined types, arrays/ranges of user defined types
          etc. This is extremely useful. A frequent example I come across is
          the following (using our syntax):
                        //load a file into memory
                        swapInPlace<BigToNative>(data.begin(), data.end());
          Depending on the platform, that last statement may get compiled out
          to a no-op. This is also the big reason why our version of "reorder()"
          is not unconditional but depends on the "swap type".
        - reorder(x) will work fine for integral types, however, has potential
          to cause issues for e.g. floating types. This has been discussed
          extensively during my proposal last year. Also, from my practical
          experience, having a floating point number as an input, I find
          the assembly typically includes (unncecessary) floating point
          load/store instructions. Something along the lines of e.g.:
                template<typename T_>
                T_ big_to_native(raw_memory<T_>)
                template<typename T_>
                swapped_data<T_> native_to_big(T_)
          might be better and result in better performance. Of course, those
          two function signatures don't solve the "reorder" signature, but that
          could be changed correspondingly.
        - reorder() should use platform specific instructions (such as bswap)
          for doing endian swapping, otherwise falling back on one of the (many)
          variants which are currently being discussed.
        - I think the alternatives such as little_to_big() and big_to_little()
          are also needed.
        - I would like to see a few standard traits/typedefs so that I don't have
          to keep checking pre-processor macros, such as
                typedef boost::mpl::true_/false_ platform_is_big_endian;
          or something similar.
        - While I haven't needed this myself, last year someone mentioned the need
          for a "general reorder" functionality. Rather than simply dealing with
          little/big endianness, allow the user to specify ANY byte ordering for
          the specific data size.

- I would like to see an endian_swap_iterator (and a raw_endian_iterator,
  which can iterate over unaligned data, too)

> - What is your evaluation of the implementation?

Other than the implementation of reorder() and its performance, which is
already being discussed on the list, (and the inclusion of platform specific
instructions, as per above) I would like to see endian<> to be implemented
in terms of the "free" functions. It seems strange to have two implementations
of the same task.

> - What is your evaluation of the documentation?


> - What is your evaluation of the potential usefulness of the
> library?

Very useful, long overdue in boost.

> - Did you try to use the library? With what compiler? Did you
> have any problems?

Tried the integer types last year, no problems. Didn't try the "new"
swapping functions.

> - How much effort did you put into your evaluation? A glance? A
> quick reading? In-depth study?

I spent a fair bit of time looking through the docs, the code and reading
the followup posts etc.

> - Are you knowledgeable about the problem domain?


> And finally, every review should answer this question:
> - Do you think the library should be accepted as a Boost library?

I see the library as having two pieces, both useful to different people.
        I would say conditionally accept the endian<> class if floating point
        support is added.
        I would vote no for the inclusion of the free function interfaces in
        their current form.



Boost list run by bdawes at, gregod at, cpdaniel at, john at