Re: [boost] [Review] Boost.Endian by BEman Dawes starts today

Subject: Re: [boost] [Review] Boost.Endian by BEman Dawes starts today
From: Vicente J. Botet Escriba (vicente.botet_at_[hidden])
Date: 2011-09-10 10:40:34

>
>     - What is your evaluation of the design?
The design of the endian class is a clasic integer wrapper, that respond 
to almost all the needs. No major problem with it excep:
I find the explicit conversion (constructor) and the implicit 
assignement a little bit confuising. I don't think the the explicit 
constructor make it safer.

The implicit conversion to the underlying type has the drawback that 
different endiannes will print always in native format. The library must 
overload the input and output operators, and as Beman has suggested add 
a facet and a some manipulators.

The endian class should be splited into endian_pack/buffer aware of the 
endianness and and endian integer providing the arithmetic operators.

The endianness scoped enum native should be replaced so it is defined 
depending on the endianess of the platform.

   BOOST_SCOPED_ENUM_START(endianness) { big, little, native=(big or 
little) }; BOOST_SCOPED_ENUM_END

This will have the advantage to reduce of 1/3 the number of specializations.

The endian classes should be able to take as underlying type a C++11 
scoped enum.

Any UDT providing access to the underlying integer type could also be 
good candidates as template parameters of the endian classes.

The conversion part could be improved:
* The reorder fuction should provide both reorder in place and returned.
* conversion of at least c-arrays, boost::arrays, tuples, pair, ... 
should be provided as these types are the most used while defining 
messages or binary formats.
The library should provide in addition endian conversion functions that 
have the endiannes as template parameters to make possible generic 
functions.

The use of a macro to choose between a POD implementation or not could 
introduce problems when two libraries using Boost.Endian expect that 
this macro takes different values. I don't know whether a policy 
parameter or duplicating the classes would be the best choice.

>     - What is your evaluation of the implementation?
Quite correct. As other have signaled,
* the endian class must use the reorder functions whenever possible.
* the reorder functions should use intrinsics when available and provide 
a better performance.

Some performances tests should be added.
>     - What is your evaluation of the documentation?
Short, very short.

The scope of the library should be clarified, limited to integer types, 
explaining the other builting types are not considered. Why only 
big/little endianness has been taken in account?

I would like to see a tutorial and examples sections that show how the 
library should be used.
* Use with a 3rd party UDT that doesn't have as parameter the underlying 
type
* Use with Boost.Chrono and Boost.Units
* Some examples reading a relatively complex data type (endian unaware) 
from a file, changing a field and then writing the data.

The reference section must document the constraints of the template 
parameters.

>     - What is your evaluation of the potential usefulness of the
>       library?
Very useful. I've used it and my extension in a lot of projects. It's a 
must for the Boost.Bitfields which manage with endiansess for bitfields.

>     - Did you try to use the library?  With what compiler?  Did you
>       have any problems?
I have not tried the review version. I adapted the initial version in 
Boost.EndianExt to take care of some of the requests made 1 year ago in 
this ML. So yes, I can say that I have used it.
>     - How much effort did you put into your evaluation? A glance? A
>       quick reading? In-depth study?
I know quite well the library internals since a long time. I proposed 
Beman to take in account some of the extension I did without success. 
Happy to see that other have provided the good arguments so the future 
Endian library (if accepted) will be closer to what I was looking for.
>     - Are you knowledgeable about the problem domain?
Yes, I think so.
>
> And finally, every review should answer this question:
>
>     - Do you think the library should be accepted as a Boost library?
>
No in its current state.
Once the library takes in account the requested modification (that Beman 
has already accepted) a mini-review will be necessary to improve the 
library before release.

Beman, be free to adapt whatever you need from Boost.EndianExt.

Best,
Vicente