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

From: Pavel Vozenilek (pavel_vozenilek_at_[hidden])
Date: 2004-09-26 10:43:44

"Pavol Droba" wrote:

> The review of the Smart Container library, written by Thorsten Ottosen
> starts today
>
>
I vote weak YES to accept smart containers into Boost.

In current state the library is missing:
 - small readable code examples
 - support for object factories
 - and other points bellow.

I'd collected quite a few comments on the library.
I didn't yet look into move_ptr and didn't dig
deep in the code.

/Pavel

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

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

  This would give casual reader quick overview
  whether or not to look deeper.

  The rest like author info etc may follow.

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

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

_____________________________________________________
4. docs, Introduction:

" in case of a map"

==>>

" in case of a associative containers like map"

_____________________________________________________
5. docs, Introduction: sentence

   "the objects in an exception safe manner."

   may mention what type of exception safety smart
   containers have or take from.

_____________________________________________________
6. docs, Introduction:

"fool proof pointer storage"

==>>

"pointer storage safer to use mistakes"

"Foool proof" suggests _absolute_ ideal.

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

_____________________________________________________
8. docs, Introduction:

"Usually faster than using containers of smart pointers"

This suggests smart pointers in the same design
could happen be faster. I would be really interested
when and how.

_____________________________________________________
9. docs, Introduction:

"Less flexible than containers of smart pointers"

should have example(s) when.

_____________________________________________________
10. docs, Tutorial:

the first example is extremely long.

There should something primitive and up
to 1/2 page first. E.g. farm iterators could
be omitted to keep it small.

It would be better to have few small examples
for each feature than one monster.

There should be <ul> list of features
available in this library and link
to related example.

White space should be minimized in examples.

_____________________________________________________
11. It should be reconsidered whether to name
    files as "ptr_deque.hpp" or "smart_deque.hpp"
    to reduce number of names user needs to keep
    in head while typing code.

    Dtto class names.

_____________________________________________________
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?
Is it safe to overwrite public functions of parent?
Should be noted just here before one does cut+paste.

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

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

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

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

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

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

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

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

_____________________________________________________
21. Clonable concept: is there anything similar to
     std::nothrow?

    Something as allocate_clone<std::nothrow>(p);

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

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

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

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

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

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

_____________________________________________________
27. "The containers are not Assignable"

    - Why not? Why not to call clone on asignee pointers
      and delete_clone on the overwritten pointers?

_____________________________________________________
28. "Ownership can be transferred from a container
     on a per pointer basis".

    This should be better explained, with example.

_____________________________________________________
29. docs, Extending Map example:

       m1.insert( s, new Foo );

    is this exception safe? What if string's
    copy constructor throws?

_____________________________________________________
30. docs, Terminology section, paragraph Ownership:

" smart container or a smart pointer may take
  ownership of an heap-allocated object"

"May take" ???

".. job to delete that object ..."

==>

"... responsibility to destroy that object ..."

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

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

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

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

_____________________________________________________
35. docs, section "When the stored pointer cannot
    be 0, how do I ...":

    - there may be code snippet example.

_____________________________________________________
36. docs, section "Are the pointer containers faster
    and do they have better memory...":

"The short answer is yes: ..."
==>
"Yes: ..."

_____________________________________________________
37. docs, section "What is polymorphic class problem":

    - an illustrating example could be here.

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

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

_____________________________________________________
40. review question 2: (about ptr_array): yes, should exist.

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

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

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

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

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

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

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

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

    Also <boost/smart_containers_fwd.hpp> should exist.

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

And boost/smart_container/detail/undef.hpp?

_____________________________________________________
49. clone_manager.hpp should have

#if defined(_MSC_VER) && (_MSC_VER >= 1200)
# pragma once
#endif

Similarly move_ptr headers and maybe others.

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

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

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

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

_____________________________________________________
54. ptr_array.hpp: typo in:

       throw bad_index( "'replace()' aout of bounds" );

_____________________________________________________
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

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

_____________________________________________________
56. There should be ptr_slist.hpp and
    maybe ptr_hash_XYZ variant.

    Support for future boost::unordered_map/set
    could be added too.

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

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

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

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

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

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

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

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

_____________________________________________________
66. detail/reversible_smart_container.hpp, line 373:

            else
                ;
        }

????

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

_____________________________________________________
65. details/scoped_deleter.hpp: member released_
    can be eliminated, Negative value of stored_ could
    have its function.

    IMHO more attention could be taken on
    possible optimoizations here as this class
    feels to be on performance critical path.

    Maybe the dynamically allocated array could
    be optimized away when size == 1.

    Also to avoid code bloat the generic void*
    implementation could be used as core.

_____________________________________________________
66. there's file with funny name map_iterator_old.hpp.

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

_____________________________________________________
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

_____________________________________________________
69. ptr_map_adapter.hpp:
    shouldn't operator[] be unsafe, check nothing
    fast operation and at() the safe one?

    Right now they both default to the same call
    lookup(). This is againts STL habits.

    IMHO BOOST_ASSERT + potetially unsafe operator[]
    should be used.

_____________________________________________________
69. ptr_map_adapter.hpp: should have
      #include <memory>

    because of using std::auto_ptr.

_____________________________________________________
70. ptr_map_adapter.hpp: file ends with funny

    #endif

_____________________________________________________
72. ptr_predicate.hpp:

#if defined(_MSC_VER) && (_MSC_VER >= 1200)
    #pragma once
#endif

should be realigned for aesthetic reasons.

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

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

_____________________________________________________
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 :

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

_____________________________________________________
76. Why there is no swap() between smart containers?

    Futhermore one may want operators ==, !=, <, ...

_____________________________________________________
77. detail/associative_smart_container.hpp:
    why is there the "this" in:

       this->remove( i );

    and so like?

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

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

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

_____________________________________________________
EOF