|
|
Boost : |
From: Thorsten Ottosen (nesotto_at_[hidden])
Date: 2004-09-26 14:34:22
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.
| _____________________________________________________
| 5. docs, Introduction: sentence
|
| "the objects in an exception safe manner."
|
| may mention what type of exception safety smart
| containers have or take from.
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?
| _____________________________________________________
| 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.
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.
| _____________________________________________________
| 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.
ok.
| 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.
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?
| _____________________________________________________
| 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.
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?
|
| _____________________________________________________
| 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.
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 list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk