Re: [boost] [review] Formal review of the Smart Container library starts today

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 
(smart_container).
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 
std::map.
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?

-------------------------
Reference:
"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?

POSSIBLE REVIEW ISSUES

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
- www.torjo.com/rangelib/) 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.

-------------------------
FAQ

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

No.

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

No.

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

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

Best,
John

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