[review][assign] Formal review of Assign v2 ends today
The formal review of the Assign v2 library upgrade by Erwann Rogard is scheduled to end today. Although there have been no actual reviews with formal votes, there has been useful discussion of the library, and we have chosen not to request an extension of the review. 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. I would like to thank all those who participated in the discussions, and Erwann for developing the library, and being so prompt in his responses during the review. A quick reminder of where to find more about the library: The documentation is online at: <http://svn.boost.org/svn/boost/sandbox/assign_v2/libs/assign/v2/doc/html/index.html> With the change log at: <http://svn.boost.org/svn/boost/sandbox/assign_v2/libs/assign/v2/doc/html/boost_assign_v2/apx/change_log.html> The Assign v2 source can be found in the Boost sandbox at: <http://svn.boost.org/svn/boost/sandbox/assign_v2/> or downloaded from: <http://www.boostpro.com/vault/index.php?action=downloadfile&filename=assign_v2_r72537.zip&directory=&> John Bytheway Review Manager
John Bytheway wrote:
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'm afraid I got as far as the first page of the documentation, saw the example, and decided that it was not for me. The fact that C++ is an ISO standard with multiple implementations and a decade-long gestation for new language features has both advantages and disadvantages. One of the disadvantages is that when we have an idea that would best be implemented in the core language, unless we're prepared to wait for a decade or to fork the language ("D"), we have to try to shoe-horn the feature into existence as a library. The results are often, in my opinion, syntactically unappealing. The question is, are such things better than nothing? For something as simple as assignment, I could not support anything whose meaning was not immediately apparent from the code: std::vector<int> v = { 1,2,3,99,42,0 }; // [*] std::map<int,string> m = { {1,"hello"}, {2,"world"} }; If that can't be achieved with the core language as it currently exists, then I'd rather leave it and write it out longhand; I'd prefer type a few more keystrokes today than have someone look at my code in 5 years time and not have a clue what it was doing. Of course, the fact that something is "not for me" does not mean that it is not suitable for Boost. Surely there must be users of assign v1 out there who could express an opinion? I think we need to initiate another round of "why are there not enough reviews?" discussions, which will get more people posting than any reviews ever do.... Regards, Phil. [*] Actually, even that isn't immediately apparent; is that 0 at the end a required sentinel, in the style of obective C's NSArray constructor: [NSArray arrayWithObjects: a,b,c,d,e,nil], or is it one of the elements to be assigned?
On Fri, Jun 24, 2011 at 11:56:14PM +0100, John Bytheway wrote:
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
participants (4)
-
Daniel James -
John Bytheway -
Lars Viklund -
Phil Endecott