[review] Formal review of the Smart Container librarystarts today (long) (2024)

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

• Next message: Bronek Kozicki: "Re: [boost] mingw-3.4.2 release candidate and regression tests"
• Previous message: Jonathan Brandmeyer: "Re: [boost] Python/GCC 3.4.1/Linux failures"
• In reply to: Pavol Droba: "[boost] [review] Formal review of the Smart Container library starts today"
• Next in thread: David B. Held: "[boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"
• Reply: David B. Held: "[boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"
• Reply: Thorsten Ottosen: "[boost] Re: [review] Formal review of the Smart Containerlibrarystarts today (long)"
• Reply: Pavol Droba: "Re: [boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"

"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

• Next message: Bronek Kozicki: "Re: [boost] mingw-3.4.2 release candidate and regression tests"
• Previous message: Jonathan Brandmeyer: "Re: [boost] Python/GCC 3.4.1/Linux failures"
• In reply to: Pavol Droba: "[boost] [review] Formal review of the Smart Container library starts today"
• Next in thread: David B. Held: "[boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"
• Reply: David B. Held: "[boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"
• Reply: Thorsten Ottosen: "[boost] Re: [review] Formal review of the Smart Containerlibrarystarts today (long)"
• Reply: Pavol Droba: "Re: [boost] Re: [review] Formal review of the Smart Container librarystarts today (long)"

Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk