Tom Brinkman wrote:
The review of Assignment Library library, written by Thorsten Ottosen 2003 (nesotto@cs.auc.dk) starts today(April 1, 2004)and runs for 10 days.
I think the library should be accepted, though I have some suggestions. The review below is based on reading the documentation for about an hour. First off, I think such a library is usefull. Besides stl container initialization, it would be usefull for other libraries which want to provide convenient initialization syntax. Now, to the suggestions 1. The library expects the user to overload 'make_insertion' for his types. Now suppose I have 'class Email' which list of 'to' and 'cc' addresses.I want to initialize both using the assign library, e.g Email e; e.add_to("Winnie-the-Pooh")("Ia"); e.add_cc("Story writer"); The problem is that I can have only one 'make_insertion' for the 'Email' class. Exposing the real containers used for storing addresses is not good idea. And there might be 'add_to_address' function which does additional handling of argument, before pushing it into the container. So, it would be nice if the library allowed to specify a *function* which will do the actual insertion. 2. The library examples contain using namespace assignment; which is likely to burn users in a subtle way. Suppose: // some header namespace email { class Email {}; std::ostream& operator<<(std::ostream& os, const Email& e); } // some source #include "some header" namespace email { using namespace assignment; void internal_function() { vector<int> v; .... v << 1, 2, 3; } } The operator<< declared in the namespace will hide the one from "using namespace", and unless user is aware about this problem he might spend a lot of time trying to understand why the right operator<< is not selected. I know it costed me about 1 hour some time ago. 3. While the += operator is fine with me, I don't like the << operator. I cannot associate the conventional meaning of the operator with 'assign all' semantic, so I suggest that this operator is dropped completely. You can provide alternative syntax for assign_all: assign(v) = 1, 2, 3, 4, 5; if assignl(v)(1)(2)(3)(4)(5) has too many paranthethis. (Since comma has lower predecence than =, the first syntax will really work). 4. The operator() and other operators in insert_assigner are declared inline. Suppose vector<some_type> v; assign(v)("first", "")("second", ""); This involves call to constructor of 'some_type' and possible calls to additional constructors needed to converted char* into types that 'some_type' excepts. If operator() is inline, those calls will be inlined as well, increasing code size. Similiar problem exists because single-argument operator() accepts a fixed type, possible requiring convertion on the caller's side. I'd suggest the declare the operators outside of class declaration. This will give the compiler more freedom -- e.g. inlining only if maximum optimization level is requested. 4. Just like other revievers, I'm not comfortable with monolitic stl.hpp 5. Operator += which is declared for all possible types makes me nervous. IIRC, stl formatiing library by Terje Sletteb used some trick to avoid exposing all those operators. Something like template<template<class T, class Alloc> sequence> class helper { friend std::ostream& operator<<(std::ostream&, const Sequence& s) { ... } } // in vector.hpp template helper<std::vector>; Terje said the explicit instantination bring the friend declaration into namespace. I'm somewhat rusty on friend lookup rules, so further investigation is needed. Documentation issues 1. The example for maps uses: map<const char*, int> month; which is a bit strange, even for example, given that comparison for "const char*" does not compare string values 2. In the graph example, I'm not sure how you add edge between vertices 4 and 6, in a graph with only 5 vertices. If new vertices are automatically added, this should be documented. And BTW, what about graph which don't use ints for vertex_descriptor? 3. I did not really understoon 'tuple_insert_assigner' role. Why is it needed? - Volodya