[boost] Assignment library: review

From: Gennadiy Rozental (gennadiy.rozental_at_[hidden])
Date: 2004-04-10 02:42:00

Hi,

This is formal review for the assignment library.

Library present partial solution for frequent problem of collection
initialization. In spite of several implementation issues I incline to
accept this submission subject to resolution of Design Issue 1.

Design
---------------------------------------------

1. Global operator+= and operator<<
I believe defining global operator+= and operator<< is unacceptable (and it
is global, cause to be used they require to be brought into space by using
statement). I propose to remove it and introduce instead operator+= and
implementation in headers dedicated to specific stl containers like this:

    template <class T, class Alloc>
    inline insert_assigner<std::list<T,Alloc> >
    operator+=( std::list<T,Alloc>& c, const typename
std::list<T,Alloc>::value_type& v )
    {
        return insert_assigner<std::list<T,Alloc> >( c, v );
    }

and operator << in appropriate extension headers.

2. Forwarding problem
This library is affected by forwarding problem. I do not know how important
it is, but may be we should use by-value scheme used by boost::bind?

3. Library doesn't allow to create const collections.
Well, I am not sure whether or not it's doable but currently it's not
supported

Implementation
---------------------------------------------

1. insert_assigner( C& c, const value_type& v ) as well as
fixed_size_assigner( const iterator& begin, const iterator& end, const
value_type& v )  are unnecessary
2. Should we use Boost::pp to implement insert_assigner, fixed_size_assigner
e.t.c. and have configurable limit?
3. Obviously code needs to be refactored into smaller pieces. One header per
stl container. Also by default it shouldn't include boost related extensions
4. ~fixed_size_assigner
This method may throw an exception. Does it always safe to rely on
uncaught_exception call? Could we move this somehow out of destructor?
5. No function template ordering handling
Why not use usual trick with extra long parameter?
6. Why not use tuple_insert_assigner for matrix assignment? Wouldn't be
safer?
7. Promised support for MSVC 6.0 doesn't seems to work

Code
---------------------------------------------
1. Why insert_assigner needs to include <boost/call_traits.hpp>
2. What is make.hpp,template.hpp?
3. I would name insert as append, cause it actually does not support inside
the collection and this is after all is assignment library
4. Code comments needs to be removed

Tests
---------------------------------------------
1.  Tests doesn't seem to perform runtime correctness checks in most cases
or rely on assert for checks. It would be preferable to use more graceful
error checks.

Good job,

Gennadiy.