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-15 14:16:13

Le 11/09/11 22:08, Beman Dawes a écrit :
> On Sat, Sep 10, 2011 at 10:40 AM, Vicente J. Botet Escriba
> <vicente.botet_at_[hidden]>  wrote:
>>>     - 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.
> As you know, the conventional wisdom is to never write a class that
> includes both an implicit conversion constructor from another type and
> an implicit conversion operator back to that other type.
>
> If you think that for some reason this conventional wisdom doesn't
> apply here, please explain:-)
I wasn't aware of this convention. I need to think a little more about it.
>
> The usual way to deal with this in C++03 of course is to make the
> converting constructor implicit. Although it was years ago, I have a
> vague memory that I forgot to do that initially, and then either
> someone pointed it out or I got into unwanted conversion trouble when
> writing test cases.
>
> With C++11, we could instead mark the conversion operator implicit. I
> have no idea if that is a better approach, and would like to wait
> until more experience develops before using it.
>
>> 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.
> I'll make sure printing issues are covered in the docs, and make sure
> the tests cover these as actually delivered by the code.
Could you tell us where you are related to i/o?
>> The endian class should be splited into endian_pack/buffer aware of the
>> endianness and and endian integer providing the arithmetic operators.
> I'm weakly in favor of separating the concerns, but want to hear what
> others say before making any commitment.
>
> One irritation with that approach is that the cleanest way to do so is
> probably via inheritance, but in C++03 that breaks PODness. That was
> what motivated me to propose the POD relaxations that are in C++11.
> Not a big thing, but unpleasant from the implementation viewpoint
> because we need to support both 03 and 11.
I would say this is an implementation detail that even if needs 
consideration is less important than the interface.
>
>> 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.
> Interesting idea! I'll give it a try!
I've tried also a different approach using tag classes and inheritance. 
native will inherit from big or little.
>
>> 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.
> One issue is whether UDT's should "just work", or require some traits
> helpers such as a way to get at the underlying type.
>
> I've just started to run tests on UDT's with the current code. So far,
> it "just works". But that's without considering efficiency - knowing
> more may be required to get reasonable performance.
Could you show some examples?
Have you tried with scoped enums in c++11?
>
>> The conversion part could be improved:
>> * The reorder fuction should provide both reorder in place and returned.
> Interesting, but providing two ways of doing something is often
> considered the not best design practice. And the tests I've run so far
> don't indicate any efficiency concern. Once all other aspects of the
> interface and implementation are settled, let's revisit that question.
I prefer to use function returning the result if the implementation 
could ensure that it as efficient as passing the result by reference. 
providing both leverage you to ensure it.
>> * 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.
> I may be misunderstanding your suggestion, but that sounds like a
> recipe for interface bloat to me. Many other operations are commonly
> performed on the contents of boost::arrays, tuples, and pairs, but the
> providers of those operations don't usually provide any special
> support for when they are to be called with arguments that happen to
> live in these types. Could you give an example of what you have in
> mind?
I think that the difference in the approaches is that you don't worry to 
define a specific function for each UDT that needs an endian conversion, 
while I would prefer to be able to use always the same function name for 
all the UDT. But I suspect that not too much people shares my approach, 
so forget this point for the review.
>
>> The library should provide in addition endian conversion functions that have
>> the endiannes as template parameters to make possible generic functions.
> Compile time dispatch on an endianness enum was also requested in
> another review. That's fine with me, but I haven't had a chance to
> figure out the interface details.
Have you had the time to think more about this point?
>
>> 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.
> Since relaxed POD's and extended unions are now officially blessed by
> C++11, the whole POD issue goes away. Remember that the 03 non-POD
> version works fine with C++03 compilers except in unions. So the
> separate versions goe away eventually. Thus I don't want to do
> anything that enshrines two versions in the interface.
Great.
>>>     - What is your evaluation of the implementation?
>> Quite correct. As other have signaled,
>> * the endian class must use the reorder functions whenever possible.
> As long as that doesn't degrade performance!
Of course, but you should try it in order to see the result ;-)
>
>> * the reorder functions should use intrinsics when available and provide a
>> better performance.
> Sure, all other things being equal.
>
>> Some performances tests should be added.
> I've already started work on a benchmark program, although it isn't
> ready for prime time yet.
Could you show your test now so we can test on other platforms?
>
>>>     - What is your evaluation of the documentation?
>> Short, very short.
>>
>> The scope of the library should be clarified, limited to integer types,
> There have been a lot of requests for extending the library to handle
> floating point types and UDT's, and I intend to do that if it can be
> done safely.
In addition, maybe you can add that you don't take care of endian issues 
with bitfields.
>
>> explaining the other builting types are not considered. Why only big/little
>> endianness has been taken in account?
> I'll add FAQ and/or add more entries to the final docs.
OK.
> Only big/little endianness is taken into account because these are the
> only endian schemes that have any practical value. All the others are
> just historical curiosities.
>
>> I would like to see a tutorial and examples sections that show how the
>> library should be used.
> A tutorial is planned, as are more examples.
>
>> * Use with a 3rd party UDT that doesn't have as parameter the underlying
>> type
> See above. That seems to be working already. The issue is nailing down
> and documenting the template parameter requirements.
Could you tell us a little bit more how this work safely?
>> * Use with Boost.Chrono and Boost.Units
> Hum... Why wouldn't it already work with Boost.Chrono and Boost.Units
> types that are integers?
It works. Just I consider that both usages show the library can be used 
well in these contexts. Boost.Bitfield uses it in the same way.

>
>> * Some examples reading a relatively complex data type (endian unaware) from
>> a file, changing a field and then writing the data.
> Yes! That's the example I have in mind for the tutorial.
Have you choosen already the data type? Could you show us, before the 
review ends, how the library will take care of conversions?
>> The reference section must document the constraints of the template
>> parameters.
> Yes!
>
> 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.
> No problem.
It would be great if you had time to give some details of how you see 
the evolution of your library before the review ends, otherwise we will 
have too much things to discuss/review during the the mini review.

Best,
Vicente