Re: [Boost-users] [boost] [review][assign] Formal review of Assign v2 ends today

Subject: Re: [Boost-users] [boost] [review][assign] Formal review of Assign v2 ends today
From: Lars Viklund (zao_at_[hidden])
Date: 2011-06-27 04:57:29

On Fri, Jun 24, 2011 at 11:56:14PM +0100, John Bytheway wrote:
> The formal review of the Assign v2 library upgrade by Erwann Rogard is
> scheduled to end today.
>
> Despite the end of the review, please feel free to discuss the library
> further, make any final comments, or even submit a late review.  I will
> certainly not declare the review request before next Monday, and will
> incorporate any comments made before then.

Hopefully this is not too late to be counted as a proper mini-review.

The test suite for the library fails miserably on VisualAge C++ 11.1, as
can be seen in the logs at [1], which doesn't bode well for a newly
written library aimed for inclusion in Boost.

The test suite fails similarly on SunStudio 12.1 on Solaris 10, with the
results seen in [2].

As a comparison, the tests for assign_v1 passes with flying colours with
VisualAge. Having the replacement library support way fewer platforms in
Boost sounds very strange to me, and is alone grounds for not including
the library.

(I was not able to complete building the v1 tests on the Solaris box I
had, as the linker seems slightly broken, something that doesn't affect
the v2 tests, as none of them can compile.)

The performance metrics seem to lack a very important metric - namely
compile-time costs in memory usage and compile time increases. The
documentation mentions compile-time costs briefly in lamenting that the
test suite takes several minutes to build, which isn't horribly
encouraging.

A library that is intended to be easily dropped into any TU you might
need it in should be very cheap to use. I currently avoid using Karma/Qi
even for things where they would simplify things, solely due to the cost
of including them.

Regarding the code, I'm a bit concerned about the namespace layout.
The current setup will cause any users of 1.0 that also happen to
include 2.0 indirectly to end up with namespaces 'v2' and 'ref' injected
wherever they previously did 'using namespace boost::assign;'.

As for naming of the functions and types, I find them both opaque and
overly generic.

'put()' has a strong risk of being used already in user code, and things
like 'csv()' are utterly meaningless without having read the
documentation, especially with regard to the non-type template argument.

The use of asdf_aux namespaces seems a bit off, considering that the
canonical naming for implementation detail is just that, 'detail'.

This library feels misguided, trying to be something the name doesn't
indicate it should be.

I found assign_v1 to be an excellent library for quick and dirty
initialization of sequences. The whole transformation pipeline that v2
introduces feels out of place and most probably belongs in a quite
different library. The documentation even mentions Boost.Range's
sequence adaptors - which makes me wonder if they're not better off
integrated with Range?

As for the tutorial, which is the closest thing to examples I find, they
seem overly terse and seems to assume that the concepts used in the
library are already known.

My vote for this library is: NO.

[1] http://www.acc.umu.se/~zao/assign_v2/vacpp-test-20110627-1014.txt
[2] http://www.acc.umu.se/~zao/assign_v2/suncc-test-20110627-1044.txt

-- 
Lars Viklund | zao_at_[hidden]