Tom Brinkman wrote:
> The review of Assignment Library library, written by
> Thorsten Ottosen 2003 (nesotto(a)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
