[boost] Re: [review] Formal review of the Smart Containerlibrarystarts today (long)

26 Sep 2004

      Hi Pavel,

Thanks for your review.

| I vote weak YES to accept smart containers into Boost.

sounds good :-)

| In current state the library is missing:
|  - small readable code examples

ok, what do you find particular non-readable about all the examples in the
example section?

|  - support for object factories

ok, I'm 100% how you imgine they should look like, so please give some
examples.

| 0. Name of the library should be "Smart Containers"
|    (note the plural).

ok.

| _____________________________________________________
| 1. It would be nice the very first sentence of docs
|    being something as:
|
| "Smart Containers are STL compatible containers
|   holding and owning object pointers."

agree, although I not sure what "xompatible" should mean.

| _____________________________________________________
| 2. docs: the link to license file may be better local.

ok, why?

| _____________________________________________________
| 3. docs, Introduction: the reference to shared_ptr
|    may be hyperlinked.

agree.

| _____________________________________________________
| 4. docs, Introduction:
|
| " in case of a map"
|
| ==>>
|
| " in case of a associative containers like map"

well,  in case of a set, it is the key_type that is allocated, not the
mapped_type.

the issue here is that std::container<T*> is not exception-safe, ie, will not
even give the
basic exception-safety guarantee. So exception-safety means at least the BESG,
possibly more.
The no one policy for all functions in the entire library.

| _____________________________________________________
| 6. docs, Introduction:
|
| "fool proof pointer storage"
|
| ==>>
|
| "pointer storage safer to use mistakes"
|
|
| "Foool proof" suggests _absolute_ ideal.

he true, this "fool proof" is impossible in C++.

| _____________________________________________________
| 7. docs, Introduction:  Assignable and
|    Copy Constructible could be hyperlinked.

ok, don't people know this?

The intended "suggestion" is that it might not matter to what you're doing,
but it might.
The test  file pointainer_speed.cpp does some simple test cases...some show
quite
large releative differences on my compilers, it might be different on others
etc. Maybe
you can get a clue about performance from that test.

Smart pointers could be faster if you wanted to *share* individual objects,
because
you can't really do that with these containers. then again, insome situations,
you could simply do
shared_ptr< ptr_vector<T> > instread of vector< shared_ptr<T> >.

| _____________________________________________________
| 9. docs, Introduction:
|
| "Less flexible than containers of smart pointers"
|
| should have example(s) when.

ok, let me explain that I view the sharing proporty of shared_ptr as a
flexiiblity because
you don't have to worry about lifetime issues. so in that sense these guys are
less flexible.

ok.

ok, I think Pavol should do a vote on that if the library gets accepted.

|
| _____________________________________________________
| 12. docs, example in section "Extending a Sequence":
|
| template< class T >
| struct my_ptr_vector :
|     public boost::ptr_sequence_adapter< std::vector<T*> >
|
|
| is the class safe against downcast and then delete?

no. it does not have virtual destructor. I guess that suggest that we should
give
ptr_sequence_adqater and all the others protected cons/de-structor.

| Is it safe to overwrite public functions of parent?

depends on what safe means. there's no virtual functions.

| Should be noted just here before one does cut+paste.

agree.

| _____________________________________________________
| 13. Clonable concept: it is question whether the function
|     allocate_clone() shouldn't be named new_clone(),
|     being in sync with delete_clone().
|
|     Other possibility could be allocate_clone/dellocate_clone.

yes, another candiate for a vote for Pavol.

| _____________________________________________________
| 14 Clonable concept: would it make any sense to provide
|    support for placement new/delete also?

I can't answer that question since I have never had the need to use it. I hope
other reviwers can anser it.

| _____________________________________________________
| 15. Clonable concept implementation:
|     shouldn't there be try/catch in:
|
|     template< class T >
|     void delete_clone( const T* t )
|     {
|         checked_delete( r );
|     }
|
|    to enforce the constraints?
|    At least it would be useful to have it
|    in debug mode to report violation.

but then again, shouldn't it then be in checked_delete()? In some sense I feel
it
beyond a library to cope with "fools" :-)

| _____________________________________________________
| 16. Clonable conceptL could there be type traits
|     is_clonable() for metaprogramming?

yes, I guess so, and I could use it internally to make a check too.

| _____________________________________________________
| 17. Clonable concept docs: there's link named
|     "(see the beginning of <a>Example 9</a>)
|     which is unclear. It should point to code.

ok.

| _____________________________________________________
| 18. Clonable concept docs: example how to employ ADL
|     and what to do it not present should be added.
|     Not every user may  be well familiar with the
|     technique.

ok.

| _____________________________________________________
| 19. docs aestetics: maybe major sections could
|     be separated by <br>.

yes, agree

| _____________________________________________________
| 20. Clonable concept: is there any requirement on
|     *a_object == *a_cloned_object?

I guess the requirement could be tightened to say as above iff
there is an operator==(). I'm not sure its useful to say so, so why add it?

| _____________________________________________________
| 21. Clonable concept: is there anything similar to
|      std::nothrow?
|
|     Something as allocate_clone<std::nothrow>(p);

If people want it there could be. However, we don't want 0 pointers in the
containers, so
what would be the point of it?

| _____________________________________________________
| 22. Clonable concept: would it be possible to add
|     support for object factories? Such thing may
|     be useful e.g. for COM or CORBA objects.

yes, how do they look like?

| _____________________________________________________
| 23. Clone Manager Concept docs: sentence
|
|
| "second property allows users to apply custom
|  allocators/deallocators for the cloned objects"
|
| It should be made clear whether there could be
| one or more custom allocators for cloned objects
| involved in single container instance.

ok, if I understand you correctly, I guess there could be:

1. the allocator used behind the scene in allocate_clone()
2. the allocator use in the underlying container

| _____________________________________________________
| 24. Clone Manager Concept: there are quite a few
|     requirements "must not throw".
|
|     Shouldn't ther be expressed in terms of T?
|     E.g. Throws only if T constructor throw.

not sure why. onthe the destructor says "don't throw" + includes some form of
T. but it is the deallocation
rutine that should not throw, nothing on T .

| _____________________________________________________
| 25. Clone Manager Concept docs: there should be short
|     code snippet showing what Clone Manager could
|     be used for.

ok.

| _____________________________________________________
| 26. Clone Manager Concept docs: it should be explained
|     what is 'view_clone_manager' good for, with
|     example, and how it affects pointer ownership.

there is a link to an example...is that a bad example?

|     Maybe it should be reconsidered whether it fits
|     well within smart container concept.

yes, the reason I have it is because it comes for free given the existing
framework.

| _____________________________________________________
| 27. "The containers are not Assignable"
|
|     - Why not? Why not to call clone on asignee pointers
|       and delete_clone on the overwritten pointers?

basically because it would be a deviation from the semantics of the underlying
type:

1. Standard containers are copyable and require copyable types
2. smart containers are clonable and require clonable types

consider also that cloning a smart container xan be extreamly expensive...it
deserves
more exposure that a "=".

| _____________________________________________________
| 28. "Ownership can be transferred from a container
|      on a per pointer basis".
|
|     This should be better explained, with example.

ok, what is missing from the example in the link?

| _____________________________________________________
| 29. docs, Extending Map example:
|
|        m1.insert( s, new Foo );
|
|     is this exception safe?

yes.

| What if string's
|     copy constructor throws?

it is not called. the prototyoe of insert looks like this:

insert( key&, T* )

| _____________________________________________________
| 30. docs, Terminology section, paragraph Ownership:
|
| " smart container or a smart pointer may take
|   ownership of an heap-allocated object"
|
|
| "May take" ???

yes, it depends on the clone_manager. if you use the view_clone_manager there
is no ownership.
I should make a footnote.

|
| ".. job to delete that object ..."
|
| ==>
|
| "... responsibility to destroy that object ..."

yes :-)

|
| _____________________________________________________
| 31. review question: "Should we remove rbegin()/rend()?
|     Boost.Range makes these unnecessary."
|
|
|     No. One may want to use:
|
| #ifdef XYZ
|     typedef ptr_vector<AClass> my_vector_t;
| #else
|     typedef vector<shared_ptr<AClass> > my_vector_t;
| #endif
|
|    and this requires exactly the same interface.

yeah, I think this substitution is wishfull thinking because

1. it give different semantics
2. the interface is not compatible, just consider indirection

however, it might be possible to get

ptr_vector< shared_ptr<T> > vs ptr_vector<T>

but I haven't investigated it in depth before this review determines if there
is a reason to do so.

| _____________________________________________________
| 32. docs, References section: Herb Sutter's site can
|     be linked here.

yes, good idea.

| _____________________________________________________
| 33. The mentined "move_ptr" framweorks sounds as
|     something what could be standalone Boost library.

yep, Jonathan Turkanis is your friend here.

| _____________________________________________________
| 34. docs, Future directions section:
|     it is not clear (to me) what role the
|     'observe()' function has. What idiaom does it support?

ah, yeah, simply that you want to keep a pointer into a collection of
pointers to "observe/change" that value.

| _____________________________________________________
| 35. docs, section "When the stored pointer cannot
|     be 0, how do I ...":
|
|     - there may be code snippet example.

even though you can see the article at Kevlin's cite?

| _____________________________________________________
| 36. docs, section "Are the pointer containers faster
|     and do they have better memory...":
|
| "The short answer is yes: ..."
| ==>
| "Yes: ..."

yes :-)

| _____________________________________________________
| 37. docs, section "What is polymorphic class problem":
|
|     - an illustrating example could be here.

yeah, I'll try to think of something. In general it is not hard to see
OO does not feel like a first class experience in C++.

| _____________________________________________________
| 38. docs, section "Why are inserting member function
|     overloaded for both pointers and references?
|     REVIEW QUESTION: do we want this behavior?"
|
|    - IMO no. Very confusing.

ok, noted. there is one motivation for map/set: here we can check if the
object already
exists and hence avoid a cloning in those cases. could be a big imrpovement.
It will also make it slightly
easier to go from

vector<string> to ptr_vector<string> and vice versa

if it would make sense.

|
| _____________________________________________________
| 39. docs question: "Which mutating algorithms are safe
|     to use with pointers?"
|
|
|     Maybe overloads of std::unique etc could be added
|     to avoid accidental misuse. Something like:
|
|    namespace std {
|
|       template<...>
|       xyz unique(boost::ptr_vector<T>::iterator, ...);
|
|    }
|
|    At least there should be exhaustive list of
|    nnt-supported std and Boost algorithms.

ok. the above is not possible since we cannot do partial sepcialization of
function templates.

| _____________________________________________________
| 41. review question 6: (about using Range): why not both?

the iterator version can be emulated by make_iterator_range( ... );
but hey, I don't mind both

| _____________________________________________________
| 42. review question 8: (const_begin/const_end): no.
|     Have an example with Range for what is useful.

ok.

| _____________________________________________________
| 43. review question 9: (key iterator): why is such
|     thing needed for? map::iterator->first isn't enough?

you have to use transform_iterator or manually code a loop to do something as
simple as
the copy() operation. I find that I have wanted it a couple of times, and that
the other alternatives
are daoable, but irritating.

| _____________________________________________________
| 44. review question 11: rather no, feels as
|     hole into type system.

ok.

| _____________________________________________________
| 45. Will there be support for circular_buffer library
|     at some time?

at the circular buffer review I explicitly asked for exception-safety
guarantees of the buffer to
enable it to become a smart container, however, I assume the is less need for
it compared to
a value_based interface. I think ptr_deque<> will make for quite fast queues
in this domain.

If there is a genuine need, it would be relatively easy to add,  me think.

|     I would ask for Multi Index but don't know enough
|     to judge its feasibility.

yeah, let's get this baby accepted first. The view_clona_manager do provide
some means on which it is faily
easy to get different interfaces.

| _____________________________________________________
| 46. docs, "Library headers" section: the heareds should
|     be hyperlinked for convenience.

ok.

| _____________________________________________________
| 47. There should be <boost/smart_containers.hpp>
|     (note its location and plural at the end).
|
|     Also <boost/smart_containers_fwd.hpp> should exist.

yeah, maybe. there is really several ways we can make traits in. for example

struct X
{
   typedef int x_type;
};

will allow us to identify X without including any header.

| _____________________________________________________
| 48. What is boost/smart_container/template.hpp for?

for me when I made a new header.

| And boost/smart_container/detail/undef.hpp?

to be scrapped.

| _____________________________________________________
| 49. clone_manager.hpp should have
|
| #if defined(_MSC_VER) && (_MSC_VER >= 1200)
| # pragma once
| #endif
|
|
| Similarly move_ptr headers and maybe others.

yes

| _____________________________________________________
| 50. The directory with headers should use plural:
|     boost/smart_containers. It is in sync with library name.

ok.

|
| _____________________________________________________
| 51. boost/smart_container/detail/size_undoer.hpp:
|     shouldn't ScopeGuard be used instead of this?
|
|     There's copy of Alexandrescu's implementation
|     within MultiIndex library.

I guess it could. Currently I have removed the resize() functions from the
vector interface so it would not be needed.

| _____________________________________________________
| 52. The library should not use sub-namespace detail
|     but smart_pointers_detail or so to avoid
|     possible clashes.

yes.

| _____________________________________________________
| 53. exception.hpp: the function argument should
|     not be named 'what' as it is confusing with what()
|     function.

really? the intention is that is what you get from what(). but I don't mind
something else.

| _____________________________________________________
| 54. ptr_array.hpp: typo in:
|
|        throw bad_index( "'replace()' aout of bounds" );

thanks!

| _____________________________________________________
| 55. exception.hpp: the names bad_smart_container_operation,
|     bad_index and bad_pointer are too generic,
|     polluting namespace and and non-descriptive.
|
|     They should be changed to:
|      - smart_containers_exception_base
|      - smart_containers_invalid_index_value
|      - smart_containers_null_pointer_inserted

ok, Pavol should make a vote about this.

|
|    These exceptions should be declared in
|    <boost/smart_containers_fwd.hpp>.

what for?

| _____________________________________________________
| 56. There should be ptr_slist.hpp and
|     maybe ptr_hash_XYZ variant.
|
|     Support for future boost::unordered_map/set
|     could be added too.

yeah, I'm waiting for a boost implementation of them to appear.

| _____________________________________________________
| 57. Maybe boost::tuple could be supported as well.

too far away in my opinion.

| _____________________________________________________
| 58. The documentation could contain result of
|     a bechmark, how faster is access via ptr_vector
|     copmpared to vector<shared_ptr> for example.

yes, agree.

| _____________________________________________________
| 59. clone_manager.hpp: in delete clone BOOST_ASSERT
|     could be added:
|
|     template< class T >
|     void delete_clone( const T* r )
|     {
|         BOOST_ASSERT(r);
|         checked_delete( r );
|     }
|
|     There are for sure many other places.

yeah, perhaps.
| _____________________________________________________
| 60. source code refers to old name of the library
|     in comments:
|
| /**
|  * Pointer-Container Library
|
| _____________________________________________________
| 61. Source should be cleaned up from remnants like:
|
| //#undef BOOST_FORWARD_TYPEDEF
| //#undef BOOST_SMART_CONTAINER_RELEASE_AND_CLONE
|
| in ptr_list.hpp.

yes.

| _____________________________________________________
| 62.ptr_list.hpp:
|
|     template< typename T, typename A >
|     inline ptr_list<T,A>* allocate_clone( const ptr_list<T,A>& r )
|     {
|         return r.clone().release();
|     }
|
|
| could clone() fail and return 0? What if there's
| special allocator returning 0 instead of throwing?

that is not allowed in the clonable concept. I should make this more explicit
in that concept.

| _____________________________________________________
| 63. The source code surely won't suffer from having
|     some overview sections. Like what is this file
|     for, what are related files to look on etc.

it probably won't...

| _____________________________________________________
| 64. documentation should have section how to
|     'smartify' an user container, preferably with step
|      by step example.

yea, agree.

|
| _____________________________________________________
| 65. details/scoped_deleter.hpp: there seems no need
|     for scoped_array here (only to bloat executable
|     and slower compilation).

hm...I need to manage a resource and I prefer not to do so manually.
I canøt imagine that scoped_array<> afffects the executable at all.

| _____________________________________________________
| 66. detail/reversible_smart_container.hpp, line 373:
|
|             else
|                 ;
|         }
|
|
| ????

else nothing. just as it is documented in the standard. But I have not
included resize so this code is to be deleted.

| _____________________________________________________
| 67. It would be nice to support systems without
|     enabled exceptions (BOOST_NO_EXCEPTIONS).

this is harder than it seems because I would have to check return values for
0, right?

yeah, I disagree, but It is really hard to do optimzations without data to
back that up.
So I'll better wait till that data exists. Moreover, the big time consumer is
heap-allocations, not
much else.

| _____________________________________________________
| 66. there's file with funny name map_iterator_old.hpp.
 to be deleted.

|
| _____________________________________________________
| 67. detail/map_iterators.hpp: the extremely hard to read
|     lines like:
|
|  template< typename I, typename K, typename V > // I = original iterator, K
| = key type, V = return value type of operator*()
|  class map_iterator : bidirectional_iterator_helper< map_iterator<I,K,V>,
| std::pair<K,V>, // V*?
|                                                      std::ptrdiff_t,
| std::pair<K,V>*,     // V*?
|                                                      V& >
|
|
|   Must be rearranged.

ok.

| _____________________________________________________
| 68.
|
| Likewise in ptr_map_adapter.hpp:
|
|         typedef BOOST_DEDUCED_TYPENAME
|             smart_container::detail::map_iterator<
|                        BOOST_DEDUCED_TYPENAME Map::const_reverse_iterator,
|                        BOOST_DEDUCED_TYPENAME Map::key_type,
|                        const value_type>
|                      const_reverse_iterator;
|
|
|     I have problem
| to find what
|     is end and what
|         is beginning here

what do you suggest?

yeah, but this is a map, not a vector. There are several choices here, eg,
call at() lookup() etc.

|
| _____________________________________________________
| 69. ptr_map_adapter.hpp: should have
|       #include <memory>
|
|     because of using std::auto_ptr.

already in reversible_smart_container.hpp

|
| _____________________________________________________
| 70. ptr_map_adapter.hpp: file ends with funny
|
|     #endif

fixed.

| _____________________________________________________
| 72. ptr_predicate.hpp:
|
| #if defined(_MSC_VER) && (_MSC_VER >= 1200)
|     #pragma once
| #endif
|
|
| should be realigned for aesthetic reasons.

ok.

| _____________________________________________________
| 72. ptr_predicate.hpp:  maybe name
|     "pointed_equal_to" is better than
|     "ptr_equal_to".

yeah, this is related to issue 4. I think one class like indirected<predicate>
might be the way to go...it would
replace this entire header.

|
| _____________________________________________________
| 73. ptr_predicate.hpp:  the functors could have:
|
| struct XYZ {
| #if BOOST_WORKAROUND(__BORLANDC__, BOOST_TESTED_AT(0x564))
|      char dummy_; // BCB would by default use 8 bytes for empty class
| #endif
|      ....
|    };
|
| optimization for BCB. There's no way to
| make it smaller with this compiler and no easier.

yes.

| _____________________________________________________
| 74. ptr_set.hpp: the code as:
|
|     template< class Key, class Compare = ptr_less<Key>,
|               class CloneManager = heap_clone_manager >
|     class ptr_set :
|
| could be rearranged into readable form:
|
|
|     template
|     <
|        class Key,
|        class Compare = ptr_less<Key>,
|        class CloneManager = heap_clone_manager
|      >
|     class ptr_set :

yeah, not too bad.

|
| _____________________________________________________
| 75. There could be some static asserts to avoid one
|     trying ptr_vector<int>.

why forbid it? it might be usefull together with the view_clone_manager

| _____________________________________________________
| 76. Why there is no swap() between smart containers?
|
|     Futhermore one may want operators ==, !=, <, ...

there is. boost.operators take care of that.

| _____________________________________________________
| 77. detail/associative_smart_container.hpp:
|     why is there the "this" in:
|
|        this->remove( i );
|
|     and so like?

to find remove() during two-phase lookup. this is required by the standard

| _____________________________________________________
| 78. will ptr_list<...>::sort() work?

yes, eventually. it's related to the algorithm issue.

| _____________________________________________________
| 79. In debug more there could be check that
|     inserted pointer is really unique.

yes.

| _____________________________________________________
| 80. Function
|
|     template<class Container, class T, class SmartContainer>
|     Container<shared_ptr<T> >
|       boost::convert(SmartContainer<T>& cont);
|
|    may come useful sometimes.
|
|    ptr_list<X> cont1;
|
|    vector<shared_ptr<X> > cont2 =
|       convert<vector>(cont1);
|
|   E.g. for "legacy" (= pre-smart container) libraries.

ok.

Thanks for your review. Hair-raising as ususal :-)

-Thorsten

[boost] Re: [review] Formal review of the Smart Containerlibrarystarts today (long)

Thorsten Ottosen