On Sun, Jan 25, 2015 at 9:19 PM, Jeremy Maitin-Shepard
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