Re: [boost] [Review] Boost.Endian mini-review

Subject: Re: [boost] [Review] Boost.Endian mini-review
From: Jeremy Maitin-Shepard (jeremy_at_[hidden])
Date: 2015-01-25 21:19:13

I have made use of this library over the past 6 months.  It has served my
needs very well, and I think it is definitely ready to be added to Boost.

I have a few comments, though:

conditional_reverse and conditional_reverse_inplace should default the
second order value to native (for both the compile-time order and runtime
order versions).  Conversions are almost always to/from native endianness,
and it is annoying to have to specify that explicitly.

The supplied endian_arithmetic and endian_buffer templates are also a bit
inconvenient to use due to the need to specify both the integer type and the
number of bits.  It would be more convenient to be able to just specify the
number of bits and have the integer type be determined automatically.  I
suppose there is the issue of needing to choose between an exact type and a
possible faster type.

For both the buffer and arithmetic types, if the aligned types are to be
kept, the user needs to make an explicit choice about alignment.  Making
either aligned or unaligned the effective default, by giving it the
big_uint32_t name, rather than the more explicit big_uint32_ut/big_uint32_at
types, makes it easier for the user to overlook alignment issues.  Therefore
I suggest that the _t names be removed in favor of the more explicit _at and
_ut names.  Potentially, the alignment amount could also be specified as a
template argument rather than be platform-dependent.

The correct usage of aligned generally seems rather tricky.  Since alignment
is platform-specific, it seems there is high risk of inadvertantly
introducing padding where it doesn't belong.  It also seems there is a high
risk of the alignment constraints being violated when some byte buffer is
cast to the struct containing endian buffer/arithmetic types.  It seems
about the only guaranteed safe use of the aligned types is if a single
endian type is declared as a local variable.

There is also the fact that on x86/x86-64, there are no separate
instructions for aligned vs. unaligned data, the code just runs a bit faster
if the data is aligned.  I think the Endian library partially takes
advantage of this (e.g. via the patch I submitted), though there may be some
places in which it does not take advantage of this.  The benefit of using
aligned types is particularly small, therefore, on x86.  It would be nice if
there were a way of declaring a "preferred" alignment, that will be used if
possible for automatic variables, but will never result in padding being
added.  Unfortunately I don't know of any way to get that behavior.