[boost] Re: Assignment library: review

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-04-10 05:21:32

Hi Gennadiy,

Thanks for your review.

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

won't boost::ref <> "solve" this. (don't bind use &?)

> 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

It has already been suggested to support

const vector<int> v = init( v )(1)(2).

> 1. insert_assigner( C& c, const value_type& v )
(a)
>as well as
> fixed_size_assigner( const iterator& begin, const iterator& end, const
> value_type& v )  are unnecessary
(b)

(a) is used with operator+=()
(b) is used with operator<<(). If this goes away, it will sill eb used in eg

assign_all( array ) = 1,2,3;

> 2. Should we use Boost::pp to implement insert_assigner,
fixed_size_assigner
> e.t.c. and have configurable limit?

Some have wanted this. I have no problems with it besides I can't see what
its good for.
Having a function/constructor with more than X arguments (x small) is hardly
good practice.

> 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

ok.

> 4. ~fixed_size_assigner
> This method may throw an exception. Does it always safe to rely on
> uncaught_exception call?

No, it is not always safe, but the unsafe stuff happens if you use it in a
destructor; not very
likely. Sutter has an article about it.

>Could we move this somehow out of destructor?

Yes. I would prefer to default initialize the remaining elements. It do put
that additional
requirement on the suported types, but not a big thing IMO.

> 5. No function template ordering handling
> Why not use usual trick with extra long parameter?

Sure. What's the trick and what's the issue?

> 6. Why not use tuple_insert_assigner for matrix assignment? Wouldn't be
> safer?

No. A matrix is usually a fixed sized container,  eg a 4-by-4 matrix. Thus
we can
make bounds checking. tuple_insert_assigner is used for forwarding to
functions that
take a "tuple" of arguments. Just plain forwarding.

> 7. Promised support for MSVC 6.0 doesn't seems to work

It's been a while since I did the test. I won't promise vc6 support
with the new proposed changes, but I will try.

> 1. Why insert_assigner needs to include <boost/call_traits.hpp>

It don't. Thanks.

> 2. What is make.hpp,template.hpp?

just rubbish.

> 3. I would name insert as append, cause it actually does not support
inside
> the collection and this is after all is assignment library

good idea.

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

Yes. Tom wanted real test too.

> Good job,

thanks.

br

Thorsten