Re: [boost] [Endian] Review / Comments

Subject: Re: [boost] [Endian] Review / Comments
From: Beman Dawes (bdawes_at_[hidden])
Date: 2011-09-08 11:50:48

On Thu, Sep 8, 2011 at 6:33 AM, Tim Blechmann <tim_at_[hidden]> wrote:
> hi,
>
> documentation
> the documentation in general is pretty short, but sufficient: the concepts of
> the library are easy, so there is no need to go into more details.
> however i would prefer if the documentation uses quickbook/boostbook. most of
> the recent boost libraries have a very similar layout of the documentation,
> but the boost.endian docs do not follow these conventions.

quickbook/boostbook has matured, so I'll take a look. But no promises.

> also the docs for `conversion functions' and for `integer types' are separate.
> personally i'd prefer if they could go into the same document, maybe having a
> structure like: introduction, integer types, conversion functions, examples,
> reference

A lot depends on what the conversion functions look like coming out of
the review. It may make sense to integrate them much more tightly, but
again, no promises.

> (preferably doxygen-generated).

I'm not a fan of doxygen.

> section `experience': this section gives no insights for people who use or
> read the code. it mainly tells: "we are not the first and the domain of the
> library is important.". imo this section can be removed (maybe the part that
> it is not related to any c library can go to the `design considerations'
>
> section "motivating use cases": this is more a marketing blurb/testimonial.
> maybe this could be changed to a section about possible use cases, listing
> `communicating between different devices' and `reading/writing of binary file
> formats'.

Added to do list.

> provided files
> i suppose, the msvc project file will go away when the library is merged into
> trunk.

I leave the msvc project directory in libs/lib-name/test (see
Filesystem, System, etc) since it needs to be under version control.

> besides, i'd suggest to move "boost/endian/support/timer.hpp" to
> "lib/endian/support", because it only seems to be used by the benchmarks.

Timer stuff isn't part of the library and wasn't included in the zip file.

> testsuite
> scoped_enum_emulation_test.cpp ... not related to boost.endian, please remove

Will do.

> endian_operations_test.cpp and endian_in_union_test.cpp ... maybe rename from
> _test.cpp to _compile_test.cpp? they don't seem to do any run-time tests. they
> also should not include <cassert> since no assertion statement is needed, this
> might speed up the compilation time of the testsuite by something like 50ms ;)

Will do.

> conversion functions
> personally, i find the interface for the conversion functions rather odd, as
> they don't actually return a value, but take a reference to the return value.
> so they cannot really be used in a functional style.
>
> if the reorder functions would have a signature like:
> int16_t reorder(int16_t x);
> it would be possible to use it as unary operator e.g. in with std::transform
> and the like. this would also avoid the need for both in-place and out-of-
> place functions.

Phil Endecott and others have made the same points, and I'm working on
a prototype right now.

> performance considerations  ...

I'm actively working on performance improvements for the conversion
functions.  At the moment, Pyry Jahkola's suggested implementation is
fastest, but I haven't tested yet with GCC.

...
>
> What is your evaluation of the potential usefulness of the library?
> very important, very useful
>
>
> How much effort did you put into your evaluation? A glance? A quick reading?
> In-depth study?
>
> roughly 3 hours, reading code, documentation, running some benchmarks.
> besides, i'm using an older version of the library for about 2 years without
> having any problems.
>
>
> Are you knowledgeable about the problem domain?
> yes
>
>
> Do you think the library should be accepted as a Boost library?
> the library should be accepted, if
>
> (a) the interface of the conversion functions is changed
> (b) the performance can be improved
> (c) the documentation integrates better with the rest of the boost
>    documentation.

Thanks for all the detailed comments!

--Beman