[boost] Re: Formal Review: Assignment Library

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-08 06:04:24

Hi Vladimir,

Thanks for your comments.

> 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.

I not sure why a constructor taking both a 'to' and a 'cc' cannot work for
you
in this case. But I see that the requirement of having more than "insert"
choice might be useful.

> 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.

So you would like something like

Email e = insert_with< &Email::add_cc >( e )( ... );

? Or perhaps

Email e = insert_with( bind( e, &Email::add_cc ) )(...);

?

> 2.  The library examples contain
>
>    using namespace assignment;
>
> which is likely to burn users in a subtle way. Suppose:
[snip example]
> 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.

Thanks for pointing this out. I'll change 'using namespace' to a namespace
alias
and make a warning about this behavior in a footnote.

> 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;

So you mean assign_all( v ) = 1,2,3,4,5; Seems ok. I guess if there is
agreement about
removing operator<<, the name hiding issue goes away for that part (but not
for operator+=())

> 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.

I did not know there was a difference. Could you point to the place in the
standard, please
(in particular, I couldn't find anything in 8.3.5 and 9.5 that supports it).

> 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

true. It is probably better not to teach programmers this.

> 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?

I think you hit a soft spot. As has been discussed in the "Forced client
complexity" thread,
I stumpled into that complexity. The thing is that I would like to hear
people's apinion about
putting these things in this library or not. If they should be in this
library, I would like
individual library authors to help.

> 3. I did not really understoon 'tuple_insert_assigner' role. Why is it
needed?

Basically because it does not call any constructor, but simply forwards a
tuple of arguments
to your callback function. (It could be explained better). This is how I
call add_edge() in BGL
even though add_edge() takes 3 and 4 arguments.

br

Thorsten