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@acc.umu.se