[boost] [Endian] Review

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_>)
                
                and
                
                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?

Fine.

>      - 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?

Reasonably.

> 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.

Thanks!

Tom