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:
> 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,
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
Added to do list.
> provided files
> i suppose, the msvc project file will go away when the library is merged into
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.
> scoped_enum_emulation_test.cpp ... not related to boost.endian, please remove
> 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 ;)
> 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?
> 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
Thanks for all the detailed comments!
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk