"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