Boost logo

Boost :

From: John Torjo (john.lists_at_[hidden])
Date: 2004-10-02 13:16:17

> * What is your evaluation of the design?

The design is ok. I'm still wondering if we really need two pairs of
iterators (begin()/end() and ptr_begin()/ptr_end()).

Also, I don't like the fact that you can't have null pointers but I can
live with that ;)

> * What is your evaluation of the implementation?

A quite 10-15 minutes glance. The implementation is quite ok.

Side-note: maybe you can hide 'template.hpp' somewhere ;)

> * What is your evaluation of the documentation?

The overall aspect is "good towards very good".

A "few" notes though:

The example at "Basic Usage"

It is too big. You should focus on only what you're trying to show
Also, I see no need for the notation:

result function()
   return blabla;

when it could be :
result function() { return blabla; }

Or, for bigger functions that are unimportant, you could simply say:

result function() { /* ... */ }

Changing the Clone Manager

You should first explain some of the internals, before jumping to "Clone
Managers" and such.

Extending a map

I don't understand the need for my_map class which just derives from
Why not use std::map directly?

Relative to this:
     // add constructors and assignment operators here, none needs
     // to be defined

What do you mean - they don't need to be defined? If I add a constructor
or so, I should define it somewhere. Otherwise, what's the point of
declaring it?

Also, I assume the #include <string> and #include "foo.h" should come
after the "// Usage" comment, otherwise they are misleading.

Clone manager requirements

"the Clone Manager need not be stateless, but the assumption is that it
probably will be."
Which is it? Can it be stateless or not?

"Stored elements are required to be Clonable for a subset of the operations"

By looking at the example, that makes me wonder how will you handle the
same example
for a view_clone_manager...

"Ownership can be transferred from a container to another container on a
per iterator range basis"
I wonder if a bettern name for the transfer function would be "move()".

Class reversible_smart_container

"All objects must be deallocable with CloneManager::operator()"

This seems a very "smart" way of deallocating objects. Is this a typo?


6. #Should iterator range versions of assign() and insert be replaced by
an Boost.Range version? Ie, should we have

template< class SinglePassRange >
void insert( const Range& r );

instead of

template< class InputIterator >
void insert( InputIterator f, InputIterator l );


Yes, I would prefer so.

9. #Should we add iterators for traversing the keys of a map? This would
mean we could say

map::key_iterator b = m.begin(), e = m.end();
copy( b, e, some_where );

I would not like that very much.
As a side note, Boost Rangelib (formely know as Range Template Library
- already provides means to walk through
the keys or values of a collection.

12. #should we provide member algorithms for unsafe standard algorithms
like unique() and remove?

Why would you say "unsafe"?
Maybe I'm overlooking something, but std::unique/std::remove should work
fine now.


Why are inserting member functions overloaded for both pointers and
REVIEW QUESTION: do we want this behavior?


> * What is your evaluation of the potential usefulness of the library?

Very useful.

> * Did you try to use the library? With what compiler? Did you have any problems?


> * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?

A little more than 1 1/2 hours.

> * Are you knowledgeable about the problem domain?

> And finally, every review should answer this question:
> * Do you think the library should be accepted as a Boost library?
> Be sure to say this explicitly so that your other comments don't obscure your
> overall opinion.

The library should be ACCEPTED.


John Torjo
-- john_at_[hidden]
Contributing editor, C/C++ Users Journal
-- "Win32 GUI Generics" -- generics & GUI do mix, after all
-- v1.4 - save_dlg - true binding of your data to UI controls!
    + easily add validation rules (win32gui/examples/smart_dlg)

Boost list run by bdawes at, gregod at, cpdaniel at, john at