On Thu, Sep 8, 2011 at 6:33 AM, Tim Blechmann <tim@klingt.org> 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