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