Subject: [boost] [Endian] Review
From: Tomas Puverle (Tomas.Puverle_at_[hidden])
Date: 2011-09-08 13:18:52
> - 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
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.:
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
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"
> - 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 acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk