[boost] Re: [[e-mail]: Re: Formal Review of the SmartContainers library]

From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-10-06 06:39:55

Hi Ross,

Thanks for your review.

|My review is from a non-expert user perspective.  Overall, I think the
|library should be accepted, though I have some suggested tweaks and
|improvements.

Thanks. Let's see if we can fix those issues.

| I am also unable to get a program using the library to
|compile, and that definitely should be fixed if I haven't made a silly
|mistake.

ok, did you use the review version or the older? If you send me the code, I
can take a look at it.

| There is one substantive area (incompatibility with some
| standard algorithms) that is a bit worrisome to me, and merits thought
| by someone wiser.

yeah, this is not an easy one.

|I used the library (ptr_vector, ptr_set, ptr_map, including nested
|ptr_maps) in a pre-release form in my application.  There were some
|minor compilation issues with that version, which I worked around, and
|which I assume Thorsten has fixed.

they should be fixed already.

|  My main motivations for using the
| library were that I was looking for a way to hide the implementation
| decision of whether I had containers of objects or pointers, and I had
| objects that were not copyable (and would have been expensive to copy).
| Even at that early stage, I found the documentation quite complete and
| extensive.

well, it's not  quite finish yet, but I'm glad I'm on the right track.

| The library was easy to use, and it worked.

ok, good to hear.

|COMPILATION TROUBLES
|
|I got the version of the code in the review notice, and adjusted my code
|to use it (some directory names changed).  I got a raft of errors,
|attached.

this is of course unfortunate. I would like to know if you can actually
compile
the test files that comes with the library.
.

|STANDARD REVIEW QUESTIONS
|
|documentation really bear on the design.  My major concern is that some
|algorithms are not safe to use on these containers.  It is not clear to
|me that the average user can tell exactly which algorithms there are.
|If the latter is true, and there is any way to remedy it, it would be
|very good to do so.  I think if this risky area is properly documented,
|it will be an acceptable risk.

yeah, one thing that I have discussed with Pavol is to check for these errors
in
a debug mode. I will be able to check such violations at the next member
function access. For example:

ptr_vector<T> vec;
std::remove_if( vec.ptr_begin(), vec.ptr_end(), something() );
...
vec.foo(); // this one triggers a debug check for all foo()

|  I favor a design with maximum
|compatibility with existing standard containers; it seems to me that has
|been achieved.

ok, I guess you value that we can say eg push_back( T() ). I agree that we
should be
able to so for container<T> compatibility.

| The documentation is extensive and quite good.  I have many comments,
| which I will give below.  The only point I consider really significant
| is that some of the reference material is a bit hard to navigate (I
| actually found the earlier version I looked at a bit clearer in this
| regard).

yes, that will be fixed (one problem is of course there is so much
documentation).
 I will probably use Jonathan T.'s java script tree-view, so users see a list
of sections of the docs in a left frame

| I think the current
|introduction could give more emphasis two significant advantages of the
|library. First, it can do things that aren't easily done at all with
|other methods, such as smart pointers.  Second, it is conceptually very
|simple, and provides a very nice abstraction.  (Both of these things are
|discussed early on; this is just a question of emphasis).

ok, I'll take it into consideration.

|I've spent around a day, maybe more, using the library, studying the
|docs, and writing this review.  As noted at the outset, I only went into
|the implementation details when I needed to figure out how something
|worked or debug a problem.  Offsetting this signficant amout of time is
|my non-expertise in these matters!

wow, that is quite a long time :-) Thanks for your effort.!

|DETAILED DOCUMENTATION COMMENTS

[When I snip, it doesn't mean I will ignore your comments, but take it into
serious consideration when
 working further on the docs and documentation]

|* Reference to polymorphic class problem is puzzling in two ways.
|First, the section on the problem does not seem to state the problem
|directly (apparently it is how to ensure proper clean up of objects
|when we refer to them with pointers to a base class; perhaps it is
|also the fact that one can't have a list of such types directly, but
|need to use pointers).  I had not heard of the problem and it doesn't
|seem like such a problem.

I think I experienced the problem before I heard about it.

http://pages.cpsc.ucalgary.ca/~kremer/STL/1024x768/ref2.html
http://www.oonumerics.org/tmpw00/kuehl.html

I should provide these references and improve the discription.

| Second, it's not clear to me what smart
|containers add to this particular problem, given that smart pointers
|are available.

I have used container< shared_ptr<T> > extensively and I still felt that
it didn't feel good. All the syntax is different with indirection all over the
place
and something as simple as vec.push_back( new Foo ) expanding into
vec.push_back( shared_ptr<Foo>( new Foo ) )
and different cast styles etc.

I usually can take such syntax, but I worked and do work with people that are
just plain confused by all that syntactic
overhead. For them, I think the smart pinter interface is part of the problem.

|* Calling this the "smart container" library and then using names like
|"ptr_xxx" seems an invitation to confusion.  "smart_xxx" would be more
|natural.  At this point, it may be too much trouble and possibility
|for error to change the names, even if you agree with this point.

Nothing is too late, but I don't agree on these names.

|* Still in that example, the use of "stable" got me thinking about
|stable vs unstable.  In particular a "stable_type" is not a
|polymorphic type.  I wasn't actually confused, and probably no one
|else will be, but it did generate a little cognitive interference.
|You might use "barn" instead of "stable".

yes, barn is much better.

|** I found it a bit difficult to find reference information.  First,
|the material requires a fair number of clicks to reach.  Second, it
|was not clear to me where to look for information that was not in a
|given entry.  There are several reasons for this.
|
|The "see also" heading mixes material you must look at (base classes and
|pseudo-base classes such as reversible_smart_container) and classes that
|are simply related (e.g. ptr_vector and ptr_array).  So it took me
|awhile to find the definition for auto_type used in, e.g., ptr_array.

ok, I guess I should seperate "see also" and "alternatives".

|Also, there are notations like "Exception safety: strong guarantee".
|A link or further explanation would be useful for those who aren't
|familiar with these terms.

ok.

|** The unsafe mutating algorithms, in the FAQ now, probably deserves
|more prominence, in a discussion (in the main documentation) of what
|algorithms are appropriate to use with these containers.  This item
|makes me nervous.  First, it sounds as if only an implementor of a
|library would know which algorithms actually moved things around by
|swapping.  This leaves the average user unsure of which algorithms are
|safe.  Second, it seems like asking for trouble for it to be so easy
|to cause problems.

yeah, if there is one rule in C++, then it must be "if you don't know what you
are doing, don't do it" :-)

I promise you that I will expand this discussion to include a list of safe and
unsafe standard algorithms
and that debug builds will catch other errors.

|4. Is the indirected predicates the right solution?
|It was exactly what I was looking for, so I'm inclined to say yes :)

ok, I'm very close to saying that we only need one class, say
indirected_fun<Fun>,
which could replace all of these functions. Would you consider such a class
harder to use? Eg,

instead of

ptr_set< T, ptr_greater<T> >

you had to write

ptr_set< T, indirected_fun< std::greater<T> > >

|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 );
|
|
|??
|The more this class can behave like any std::container, the better.
|So I'd lean toward leaving it as is.  However, if the Boost.Range
|thing is compatible (I'm not familar with it), this argument loses
|force.

unfortunately it is not a drag and drop replacement for functions taking two
iterators.
I do think it could make sense to provide both.

|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 );
|
|That would be useful.

I asked John Torjo to provide

copy( map_keys( m ), some_where )

in his range library. I think that would be a more generic solution.

|13. Is the shared_ptr idea described in future directions a good idea?
|I'm not sure I grasp the concept well enough to comment, but it seems
|to me it kind of muddies the waters.  That is, shared pointers are a
|different kind of ownership concept than this library, and I suspect
|it will be cleaner to keep them separate.  I mean, mostly, cleaner for
|users.

ok, noted.

Let me explain what kind of flexibility we might achieve if this was possible.

1. once you discover that you want to observe pointers in the container, you
currently
    would use a bald pointer hoping all goes well and the element is not
removed; if ptr_container<shared_ptr<T>> was
   posible, you could just change a typedef to switch between safe/unsafe

2. you get the indirected interface instead of the normal interface

3. you might switch between deep clone and shallow clone semantics of
shared_ptr's just by changing a typedef
    to use a different clone_manager.

About the code, please send it to me.

And many thanks for your detailed review. It's nice to see people from
out-side boost
are taking an interest in the libraries; I wish more would do.

br

Thorsten