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

From: Ross Boylan (ross_at_[hidden])
Date: 2004-10-06 13:45:53

I've snipped parts I'm not commenting on further.

On Wed, Oct 06, 2004 at 01:39:55PM +0200, Thorsten Ottosen wrote:
> 
> | 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.
Review version.  I just sent my code in a separate (off-list) email.

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

I realize now I might have been able to get away with smart pointers,
as I don't think they usually copy the objects they point to.  But
perhaps they would have gagged on the lack of copy c'tors?  At any
rate, the library under review was more straight-forward.  And I did
want to hide whether I was using objects or pointers to them; again,
this library uniquely meets that need.

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

I assume that I can't, as the notice said they didn't work with the
release version of boost.  I'm using 1.31.

> 
> |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()

That's a neat solution.  Of course, it still requires the user to run
a build in debug mode.

> 
> |  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
It would be nice to have a non-java version as well, as that is
slightly problematic on some systems.  For example, I could make java
work with my current browser, but I haven't gone to the trouble.

In terms of the documentation design, there are at least 3 use cases:
1) someone seeking initial familiarity with the library
2) someone who wants to know about a particular container
3) someone trying to decide which exact container to use, when they
are already familiar with the library in general terms.

I was mostly thinking of 2) in my comments.  If you're very smart, you
can think of all 3!  For 2), one needs an easy way to get to the
container in question, and then an easy way to see what it inherits.

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

You're welcome.  I figured it was the least I could do in return 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

Thanks; those are good references.

> 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

Good point.

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

Do you agree that having the containers in the "smart container"
library called "ptr_xxx" is a bit confusing?

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

How about "inherits from" or "protocol from" instead of "see also"?

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

Can the safe and unsafe algorithms be know a priori, without knowing
the implementation of the library?

> 
> |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> > >
That's fine.

I thought if I set ptr_set<T> I automatically got the indirected
comparison.  Would that be the case under either approach?  Or have I
forgotten how it works?

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

It would be good to mention that in the docs of this library.

> 
> |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
Ah, I think I get it.  You share the pointer, but you still get the
benefits of automatic indirection.  
> 
> 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.
> 
Yes, that probably would be worthwhile.

One other point: in material you snipped, I advocated returning an
iterator and throwing an exception if something goes wrong.  As you
pointed out in earlier correspondence, this actually goes against the
standard library model, which doesn't throw exceptions in similar
circumstances.  Since, in general, compatibility is good, that's a
mark against that approach.  In this situation, my personal urge for
convenience outweighs it, but you certainly shouldn't break
compatibility without a conscious judgement.