Boost logo

Boost :

Subject: Re: [boost] [Review] Boost.Endian mini-review
From: Beman Dawes (bdawes_at_[hidden])
Date: 2015-01-26 09:39:52


On Sun, Jan 25, 2015 at 9:19 PM, Jeremy Maitin-Shepard
<jeremy_at_[hidden]> wrote:
> 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.

Makes sense. Will give it a try.

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

My expectation is that the class templates will only be used directly
is in highly generic code where specifying the integer type is not a
problem. I also worry about introducing change to these critical
templates at this stage in the process.

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

That's an interesting thought. Alignment is a critical choice, so
requiring it be explicit might be doing users a favor. What do others
think?

> Potentially, the alignment amount could also be specified as a
> template argument rather than be platform-dependent.

I'd rather not introduce additional complexity for something that is a
potential rather than demonstrated need.

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

Agreed. Docs changed to recommend static_asserting that sizeof the
enclosing structure is correct.

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

In isolation, perhaps. But when coupled with software engineering
practices like use of asserts and static asserts, aligned types are
safe.

> 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),

Yes, your patches have been applied:-)

> though there may be some
> places in which it does not take advantage of this.

Possibly. Any additional patches would be welcomed.

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

I'm afraid we are getting into an area of diminishing returns,
although it is hard to know for sure without a specific proposal.

My feeling is that the library is "good enough", and that future
effort is best spent on a proposal for standardizing it:-)

Thanks,

--Beman


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk