"David B. Held"

...

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

You mean like Boost.Iterator, Boost.Thread, Boost.Algorithm, Boost.Tuple, etc.?

It contains more than one container. Similar to "Smart Pointers" library. /Pavel

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

"Pavel Vozenilek" <pavel_vozenilek@hotmail.com> wrote in message news:cj7d2c$tta$1@sea.gmane.org... | Hello Thorsten, | | > | - small readable code examples | > | > ok, what do you find particular non-readable about all the examples in the | > example section? | > | They should fit into 1/2 of page whenever possible. yes, I'm sure there will be lots of improvements to the docs before a post-review. | > | 7. docs, Introduction: Assignable and | > | Copy Constructible could be hyperlinked. | > | > ok, don't people know this? | > | Boost authors for sure. Ther are also beginners. yes, agreed. | | _____________________________________________________ | > | 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. | > | This feature can be likely accomplished when object | factories get supported ok. | | _____________________________________________________ | > | 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" :-) | > | checked_delete accepts 0. smart containers cannot. yes, so you want try { if( t == 0 ) throw something(); checked_delete( t ); } ? :-) | > | These exceptions should be declared in | > | <boost/smart_containers_fwd.hpp>. | > | > what for? | > | it is quite common. | | #include <...fwd> // no need for full declaration or to search where | exceptions are | | try { | some_func_using_some_smart_container() | } catch(smart_container_base_exception&) { | } yes, ok. | | _____________________________________________________ | > | 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. | > | There could be performance advantage not to use scoped_array. | And it is one line in destructor. yeah, I'll look into it. | > | _____________________________________________________ | > | 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? | > | I think no. Failed allocation should terminate application | in such case. ok, that makes it easy. | > what do you suggest? | > | Alexandrescu's style? yes, ok. | One more question: | | - if I have polymorphic hierarchy, will the allocate_clone(T*) | clone the dynamic type of class or its static type? it's up to you. If you let it return ptr->clone() which is virtual, then its virtual ortherwise its not. So in the examples from the toturial, animals are cloned virtually. | I ask because | there's semifinished library (polymorphic_map) in Files section | that could be used for such purpose. I took a look at this stuff. It has AFAICT nothing to do with smart containers, in ptr_map<key,T>, T will be heap-allocated. in polymorphic_map<key,T>, key might be heap-allocated. The usage for poymorphic_map seems to be very narrow AFAICT. br Thorsten

| ______________________________________________________ | > | there's semifinished library (polymorphic_map) in Files section | > | that could be used for such purpose. | > | > I took a look at this stuff. It has AFAICT nothing to do with smart | > containers, in ptr_map<key,T>, T will be heap-allocated. | > in polymorphic_map<key,T>, key might be heap-allocated. The usage for | > poymorphic_map seems to be very narrow | > AFAICT. | > | The library would allow to simulate the virtual function clone() | without need to change interface of the base class. | It could be really relevant for a special clone_manager. ok, I can't imagine it, but maybe the library author can show that? | And it should be stressed Clonable concept is | mere requirement of one (default) clone manager. | Now Clonable docs comes before clone manager. ok. it is also a concept of it own. | It may be also useful to provide few more | clone managers in library: | | - the one relying on virtual T* clone(T*) member, | | - possibly the one that is able to detect dynamic type | at runtime and properly contruct clone, w/o need | for special clone support in class, hm... how would that work? | - one which allows to bind parameters to constructor | early (those parameters being added to copy constructor | first parameter). | | - maybe one for COM objects as example of using | object factory (or something simpler) | | - one using non-default allocator and then | placement new can't you do that from within T* allocate_clone( const T* ptr ) { return my_clone_func( ptr ); } ? | - one using serialization/deserialization to create | clone (the serialization process may for example | fluch internal caches or re-adjust some internal | structures) hm...the thing is that the clone manager controls all cloning, also when clones are inserted coming from other containers...seems wierd to me. br Thorsten

"Pavel Vozenilek" <pavel_vozenilek@hotmail.com> wrote in message news:cj9ndu$8s3$1@sea.gmane.org... | | "Thorsten Ottosen" wrote: | | > | It may be also useful to provide few more | > | clone managers in library: | > | | > | - possibly the one that is able to detect dynamic type | > | at runtime and properly contruct clone, w/o need | > | for special clone support in class, | > | > hm... how would that work? | > | It is possible. Exception handlers store relation between types | and such handlers could be generated by metaprogramming. | The polymorphic_map uses this trick. Ok, I can evaluate it and make it part of a post-review once somebody submit the code. | > | - one using non-default allocator and then | > | placement new | > | > can't you do that from within | > | > T* allocate_clone( const T* ptr ) | > { return my_clone_func( ptr ); } | > | > ? | > | Yes but the clong would be separated into few pieces | instead one. ok, code please :-) | > | - one using serialization/deserialization to create | > | clone (the serialization process may for example | > | fluch internal caches or re-adjust some internal | > | structures) | > | > hm...the thing is that the clone manager controls all cloning, also when | > clones are inserted coming from other containers...seems wierd to me. | > | It is weird but should work. | I use this trick somewhere, so I put it as option. Ok, now I get it. It's like in Java where you can make a fake clone by serializing the class. br Thorsten

"Pavol Droba" <droba@topmail.sk> wrote in message news:20040926194147.GX29008@lenin.felcer.sk... | In other words, only algorithms like sorting and permutations are safe. | | Thorsten, please correct me if I'm wrong. | | | I find this behaviour quite dangerous and I don't see any great benefits it brings. It's always about finding the right balance between idiot safe usage and then allowing the programmer to get the job done. If ptr_iterator is not mutable we get 1. view-clone_manager can sort what-ever it views, but not itself. 2. we cannot provide all foreseeable algorithms as members which suggest that providing member is a bad idea 3. we risk users will just do &*begin(), &*end() to get the pointers anyway br Thorsten

As I suggested, we need to rewrite only few algorithms from the std

"Pavol Droba" wrote: library and

provide an inferface, that would enable to write other algorithms in a way, that is safe.

In such case other libraries (like Range, http://rangelib.synesis.com.au/) could employ algorithms tailored to their need. /Pavel

On Mon, Oct 04, 2004 at 02:09:46AM +0200, Thorsten Ottosen wrote:

"Pavol Droba" <droba@topmail.sk> wrote in message news:20041003215617.GA6378@lenin.felcer.sk... | On Sun, Sep 26, 2004 at 10:03:17PM +0200, Thorsten Ottosen wrote:

[snip] |

| Internal swap() can check in the debug mode, that both operands are from the | same | container.

why can't you swap element from different containers?

I see, it can be other smart container, just I'm not sure if some kind of transfer mechanism would not have to be incorporated in this case.

| This would allow to easily (and naturaly) write most of the 'safe' | algorithms.

are you suggesting to rewrite all the mutable standard algorithms?

I'm not sure where are you heading? We are talking about sorts and and permutations. There are not so many of them. If they are provided as member functions, these would be one-line long. So it yeilds about 20-30 lines of code all together. Other mutating algorithms are not usable anyway. swap functionality would enable to write other 'safe' algorithms.

| | > 3. we risk users will just do &*begin(), &*end() to get the pointers anyway | > | | If somebody would do this, it is more then clear, that there is a great | potential | for something to get wrong, unlike when you write | | std::remove(vec.ptr_begin(), vec.ptr_end(), ...) | | which seems very reasonable.

not if you know what remove does..

std::remove( vec.begin(). vec.end() )

will either fail at compile time or do it right.

Are you sure about this? The knowledge about remove algorithm is far not enough to comprehend how it would work in conjuction with ptr_iterator. Especialy when it would actualy work, but the program will crash at the destruction of the smart container because of double-deletion. Regards, Pavol

On Sep 27, 2004, at 2:46 AM, Thorsten Ottosen wrote:

"Howard Hinnant" <hinnant@twcny.rr.com> wrote in message news:83B293FA-1013-11D9-9D49-003065D18932@twcny.rr.com... | container<sole_ptr<T>> will have the same overhead as that demonstrated | in the Smart Container library. A big difference will be that | iterators into the container will dereference smart pointers, and not | the pointed-to elements. I also anticipate C++0X will contain some | really slick indirect_iterator types that will transform | container<sole_ptr<T>>::iterator appropriately. These will likely be | heavily influenced by (if not based on) the Boost.Iterator Library.

I assume that there will still be some differences:

1. a large part of the in this library is indirected, eg. front() in container<sole_ptr<T>> would return sole_ptr<T>

Right.

2. release() and clone() can be made faster by moving instead of using an std::auto_ptr<> as today

Yup.

3. your map iterators would be different

Right, just smart pointers to pair<const sole_ptr<T>, U>.

4. If you do intend that the interface of container<sole_ptr<T>> has the same interface as container<T>, then you loose the domain specific interface and the Clonable metaphor

Right again.

| Assuming things go just peachy, all std::containers will be move-aware | and thus able to contain move-only types like sole_ptr<T>. Also I | would like to see all in-place-mutating std:: sequence algorithms | (remove_if, unique, etc.) rewritten to deal with ranges of move-only | types as well. Such algorithms, if not rewritten for move-only types | will fail at compile time (as opposed to run time) for move-only types.

That would be nice. If all mutating algorithms would be guaranteed to call a swap and never use a copy-constructor, I think we could make an iterator for the smart containers that was indirected *and* did "the right thing" in those algorithms.

Agreed, sort of. We don't want to mandate that all algorithms call swap. That would be too expensive for some algorithms (like remove) when operating on sequences of some types (like int or complex<T>). Rather I would like to mandate that algorithms such as remove work for movable but non-copyable types. I.e. internally where remove today might say: *j = *i; it would instead have to code: *j = move(*i); This might end up calling swap for some types, if the author of that type has implemented move assignment with swap. For other types (like int, or complex<T>), there will be no move assignment, and the above change will make absolutely no difference in behavior or efficiency (right down to the assembly that is generated). All move(*i) does is tell the compiler to treat *i as an rvalue instead of an lvalue. Then it is up to the author of *i to overload assignment on rvalues (or not).

| container<sole_ptr<T>> is not copyable nor copy-assignable. | container<sole_ptr<T>> is movable. Thus sequences of | container<sole_ptr<T>> (container<container<sole_ptr<T>>>) can also be | operated on by move-aware in-place-mutating sequence algorithms. | | Elements of container<sole_ptr<T>> can't be copied into or out of the | container with copy syntax: | | sole_ptr<T> t = v[i]; // compile time error | | But elements can be moved into or out of the container: | | sole_ptr<T> t = move(v[i]); | v.push_back(move(t));

so move(v[i]) must also erase the element and then move the rest of the vector one place back?

Nope. move(v[i]) would move from the sole_ptr<T> at i, but not erase it. v[i] would now own nothing. This is another difference between this future language I'm describing, and the Smart Container Library. The former allows null pointers where the latter doesn't.

| This future move-aware | container<clone_ptr<T>> would not clone objects as it moved them around | internally, but would clone objects for the purpose of copying them | into or out of the container,

sounds expensive to me.

<nod> If clients wanted to, they could move clone_ptr<T> into and out of the container instead of copy it in and out. For rvalue clone_ptr<T>'s, this would happen automatically, but for lvalue clone_ptr<T> the client would have to explicitly request the move. E.g.: v.push_back(clone_ptr<base>(new derived)); // rvalue clone_ptr moved into v clone_ptr<base> p(new derived); v.push_back(p); // p copied/cloned into v v.push_back(move(p)); // p moved into v, p now null -Howard

On Sep 27, 2004, at 9:09 AM, Thorsten Ottosen wrote:

From: "Howard Hinnant" <hinnant@twcny.rr.com>

| On Sep 27, 2004, at 2:46 AM, Thorsten Ottosen wrote: | | > "Howard Hinnant" <hinnant@twcny.rr.com> wrote in message

| > | But elements can be moved into or out of the container: | > | | > | sole_ptr<T> t = move(v[i]); | > | v.push_back(move(t)); | > | > so move(v[i]) must also erase the element and then move | > the rest of the vector one place back? | | Nope. move(v[i]) would move from the sole_ptr<T> at i, but not erase | it. v[i] would now own nothing. This is another difference between | this future language I'm describing, and the Smart Container Library. | The former allows null pointers where the latter doesn't.

hm...I don't get this...what happens then if I try to access v[i]? Do I get undefined behavior?

It depends on what you do with v[i]. You could (for example): if (v[i]) ... or v[i].reset(new T); or v[i] = call_func_returning_sole_ptr(); and all of that is well defined. But if you try to dereference a sole_ptr<T> which owns no object, then you fall into undefined behavior: *v[i] = t; // run time error, null dereferenced In a nutshell, you can view container<sole_ptr<T>> just like container<auto_ptr<T>>, except the former is safe from accidental moving because it won't move (from lvalues) with copy syntax like auto_ptr does. But it will move (from lvalues) with syntax that doesn't look like copy. Also container<sole_ptr<T>> will actually compile (someday) which is a really nice bonus (compared to container<auto_ptr<T>>)! ;-) -Howard

On Sep 27, 2004, at 10:20 AM, David Abrahams wrote:

Howard Hinnant <hinnant@twcny.rr.com> writes:

...

I also anticipate C++0X will contain some really slick indirect_iterator types that will transform container<sole_ptr<T>>::iterator appropriately. These will likely be heavily influenced by (if not based on) the Boost.Iterator Library.

Not if nobody else fights for that library in the LWG.

pessimistic-ly y'rs, Dave

I wouldn't be so pessimistic. I don't believe a single member of the lwg expressed the opinion that we didn't want it in C++0X. It is clear we want something like the boost iterator library. It isn't clear /exactly/ what we want. It's a complicated area, that's all. -Howard

"Howard Hinnant" <hinnant@twcny.rr.com> wrote in message news:83B293FA-1013-11D9-9D49-003065D18932@twcny.rr.com... | On Sep 26, 2004, at 10:28 AM, Pavol Droba wrote: | container<sole_ptr<T>> will have the same overhead as that demonstrated | in the Smart Container library. A big difference will be that | iterators into the container will dereference smart pointers, and not | the pointed-to elements. I also anticipate C++0X will contain some | really slick indirect_iterator types that will transform | container<sole_ptr<T>>::iterator appropriately. These will likely be | heavily influenced by (if not based on) the Boost.Iterator Library. This brings up a question: how should these iterators cope with nulls as you intend sole_ptr<T> to allow? Afterall, if the iterator is indirected it needs to assume non-null values. br Thorsten

Thorsten Ottosen wrote:

Hi John,

Thanks for the review.

"John Torjo" <john.lists@torjo.com> wrote in message news:415EF071.3040204@torjo.com... | | > | > * What is your evaluation of the design? | | The design is ok. I'm still wondering if we really need two pairs of | iterators (begin()/end() and ptr_begin()/ptr_end()).

so which would you remove?

ptr_begin and ptr_end.

| Also, I don't like the fact that you can't have null pointers but I can | live with that ;)

ok, do you find the null object pattern undesirable? I actually think it is a strength.

There are times when it's a good thing (the null object pattern). But I'm really against creating such a class for each type I want to use smart_container for. Besides, there are operations which don't make sense for a "null object". And, for each new (virtual) function you add, you should update the null_object class - which I think can turn into a maintenance nightmare :(

| --------------------- | Changing the Clone Manager | | You should first explain some of the internals, before jumping to "Clone | Managers" and such.

ah yes, the tutorial can be much better...and will be eventually.

:) look forward to it ;)

| ---------------------- | Extending a map | | I don't understand the need for my_map class which just derives from | std::map. | Why not use std::map directly?

It's beyond what I want to do myself, but the idea should be that you can if you want.

I don't understand the above phrase.

| Relative to this: | // | // add constructors and assignment operators here, none needs | // to be defined | // | | What do you mean - they don't need to be defined? If I add a constructor | or so, I should define it somewhere. Otherwise, what's the point of | declaring it?

the point is that in this approach you don't need to define more than you want; the example above requires you to define standard map contructors because the ptr_map code depends on those constructors.

Well then, say so ;) Something like "add constructors here (optional)" - or something.

| ------------------------- | Clone manager requirements | | | "the Clone Manager need not be stateless, but the assumption is that it | probably will be." | Which is it? Can it be stateless or not?

it can be and the two default ones are. I need to express this clearer.

yup.

| "Ownership can be transferred from a container to another container on a | per iterator range basis" | I wonder if a bettern name for the transfer function would be "move()".

Move sounds ok. It might be confused with move semantics though.

But it seems to be move semantics ;)

| | | ---------------------------- | Class reversible_smart_container | | "All objects must be deallocable with CloneManager::operator()" | | This seems a very "smart" way of deallocating objects. Is this a typo?

not sure what you mean. operator()() is from the clone manager is used as a deleter all over the place, internally, that is were the requirement stems from.

Yes, but for me - by reading CloneManager::operator(), I would think it would provide a clone. Thus, it's misleading. I think it would me more straightforward not to use operator() in this case. You could have a function, called 'destroy()' or something.

| 9. #Should we add iterators for traversing the keys of a map? This would | mean we could say | | map::key_iterator b = m.begin(), e = m.end(); | copy( b, e, some_where ); | | | | I would not like that very much. | As a side note, Boost Rangelib (formely know as Range Template Library | - www.torjo.com/rangelib/) already provides means to walk through | the keys or values of a collection.

ok, how does it look like?

Like this: std::map<key,value> m; std::vector<key> k; std::vector<value> v; rng::copy( transformed(pair_1st(m)), std::back_inserter(k) ); rng::copy( transformed(pair_2nd(m)), std::back_inserter(v) );

| 12. #should we provide member algorithms for unsafe standard algorithms | like unique() and remove? | | Why would you say "unsafe"? | Maybe I'm overlooking something, but std::unique/std::remove should work | fine now.

depends on what iterator you use. when using pointers these algorithms can remove pointers and duplicate others in the containers.

oh yes! I see the light now ;) You're totally right.

| ------------------------- | FAQ | | Why are inserting member functions overloaded for both pointers and | references? | REVIEW QUESTION: do we want this behavior? | | No.

ok, also if it means loss of optimality in for map/set with uinque keys?

Would you please exemplify? Thanks. Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)

| > | | > | The design is ok. I'm still wondering if we really need two pairs of | > | iterators (begin()/end() and ptr_begin()/ptr_end()). | > | > so which would you remove? | | ptr_begin and ptr_end.

isn't it easier just not to use them if you don't want?

hmmm.. ok ;)

| > | > | Also, I don't like the fact that you can't have null pointers but I can | > | live with that ;) | > | > ok, do you find the null object pattern undesirable? I actually think it is a | > strength. | | There are times when it's a good thing (the null object pattern). | But I'm really against creating such a class for each type I want to use | smart_container for.

how often do we need the nulls? (ie, would you need it for all classes?)

I can't make an estimate (which would only be subjective), but I think there are cases. Another problem, IMO, is that client code will need to be more complicated, in order to find out if a value is null or not.

| Besides, there are operations which don't make sense for a "null object".

can you give some examples?

How about, cloning ;) ? Of course, you can always make a null_object class with all functions throwing an error.

| And, for each new (virtual) function you add, you should update the | null_object class - which I think can turn into a maintenance nightmare :(

if your inheritance hierarchy contains N distinct classes, you would need to add N implementations of that virtual function anyway, right?

Not necessary... I can add one implementation, and some of those distinct N classes don't have to override it ;)

| > | ---------------------- | > | Extending a map | > | | > | I don't understand the need for my_map class which just derives from | > | std::map. | > | Why not use std::map directly? | > | > It's beyond what I want to do myself, but the idea should be that you can if | > you want. | | I don't understand the above phrase.

if you want to extend the std::map for some reason or make your own map, then you can do so and you can use that new map as a basis for a smart container too using ptr_map_adapter.

That is quite ok. But please rephrase the example or so, because from the example, tihs was quite unclear (to me).

| > | "Ownership can be transferred from a container to another container on a | > | per iterator range basis" | > | I wonder if a bettern name for the transfer function would be "move()". | > | > Move sounds ok. It might be confused with move semantics though. | | But it seems to be move semantics ;)

well, yes and no. "yes", stuff is moved, and "no" move semantics deal with rvalues by overloading copying for rvalue-references &&.

It might sound wierd that x.move() does not require T to be moveable. But let's have Pavol or me make a vote about it.

Ok. Oh, these concepts ;)

| > | > not sure what you mean. operator()() is from the clone manager is used | > as a deleter all over the place, internally, that is were the requirement | > stems from. | | Yes, but for me - by reading CloneManager::operator(), I would think it | would provide a clone. Thus, it's misleading. | | I think it would me more straightforward not to use operator() in this | case. You could have a function, called 'destroy()' or something.

I guess it might depend a bit on whether the auto_type should be exposed or not. I'd rather say move_ptr<T,CloneAllocator&> than move_ptr<T,SomeClass&> and then explaining that SomeClass calls CloneAllocator::destroy() from its operator()().

This would also require another object in the class + another indirection which might make it harder to optimize away if operator()() is empty.

I'm not sure what you mean, but I think it would be worth it. Again, I find it very misleading for CloneManager::operator() to do destruction.

| > ok, how does it look like? | | Like this: | std::map<key,value> m; | std::vector<key> k; | std::vector<value> v; | | rng::copy( transformed(pair_1st(m)), std::back_inserter(k) ); | rng::copy( transformed(pair_2nd(m)), std::back_inserter(v) );

nice :-) but what about

rng::copy( map_keys( m ), std::back_inserter(k) ); rng.:copy( map_values( m ), std::back_inserter(k) );

so we don't have to go into the pair terminology?

Of course. Just two more wrappers ;)

| > | > ok, also if it means loss of optimality in for map/set with uinque keys? | | Would you please exemplify? Thanks.

for example, consider

boost::ptr_set<T> set; boost::ptr_vector<T> vec; ... // fill vec with many duplicates as defined by operator< on T

for( 0..vec.size() - 1 ) { // #1 set.insert( vec[i] ); // #2 set.insert( allocate_clone( vec[i] ) ); }

#1 can check if vec[i] already exists as defined by operator< on T. If it does exists, there is no need to make a new clone. #2 always make a new clone and might need to delete it afterwards

I might be totally off-base here, but why not an insert_clone function? Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)

| | Another problem, IMO, is that client code will need to be more | complicated, in order to find out if a value is null or not.

I don't understand that. One of the objectives of a null object is that you don't have to worry about a value being null.

really? I thought the purpose of null was to say "there's no object here. You can't do anything with me other than compare other pointers to me."

| > | And, for each new (virtual) function you add, you should update the | > | null_object class - which I think can turn into a maintenance nightmare :( | > | > if your inheritance hierarchy contains N distinct classes, you would need to | > add N implementations of that virtual function anyway, right? | | Not necessary... I can add one implementation, and some of those | distinct N classes don't have to override it ;)

you could insert the null object class between the abstract base class and the leaf classes.

I've never seen a design like this, would never do something like this myself, and I think I would really dislike any design that exibits this.

| > | > | > | > ok, also if it means loss of optimality in for map/set with uinque keys? | > | | > | Would you please exemplify? Thanks. | > | > for example, consider | > | > boost::ptr_set<T> set; | > boost::ptr_vector<T> vec; | > ... | > // fill vec with many duplicates as defined by operator< on T | > | > for( 0..vec.size() - 1 ) | > { | > // #1 | > set.insert( vec[i] ); | > // #2 | > set.insert( allocate_clone( vec[i] ) ); | > } | > | > #1 can check if vec[i] already exists as defined by operator< on T. | > If it does exists, there is no need to make a new clone. #2 always make a new | > clone | > and might need to delete it afterwards | | I might be totally off-base here, but why not an insert_clone function?

That would be more explicit.

It might be worth to have the existing interface if users want to try changing from vector<string> to ptr_vector<string> just to see what happens.

I don't like phrases like "hey, let's change from std::vector to std::map, just to see what happens" (I know I exagerated a bit). When you change something, you should have a reason, and know what you're doing (tradeoffs, etc.). Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)

| > It might be worth to have the existing interface if users want to try changing | > from vector<string> to ptr_vector<string> | > just to see what happens. | > | | I don't like phrases like "hey, let's change from std::vector to | std::map, just to see what happens" (I know I exagerated a bit). When | you change something, you should have a reason, and know what you're | doing (tradeoffs, etc.).

Ross Boylan thought it was important.

Just in case it counts: I am against it. Best, John -- John Torjo -- john@torjo.com Contributing editor, C/C++ Users Journal -- "Win32 GUI Generics" -- generics & GUI do mix, after all -- http://www.torjo.com/win32gui/ -- v1.4 - save_dlg - true binding of your data to UI controls! + easily add validation rules (win32gui/examples/smart_dlg)

On Oct 4, 2004, at 10:22 AM, Pavol Droba wrote:

Hi Boosters,

Formal review of the Smart Containers library from Thorsten Ottosen ends tommorow. The number of reviews is rather small until now. It somebody needs more time, please declare this on the list.

I'm *very* interesting in reviewing this library, but with all of this pre-release activity I won't have a chance to review it for at least a few more days. Doug

Hi Boosters, Formal review of the Smart Containers library from Thorsten Ottosen has been extended until Saturday, October 7th 2004). Any opinions or reviews are more then welcome. Regards, Pavol Droba Original invitation message is here: The review of the Smart Container library, written by Thorsten Ottosen starts today (September 26th, 2004) and runs for 10 days (until October 5th, 2004). The latest version of the library can be found at http://boost-sandbox.sourceforge.net/smart_container.zip or http://groups.yahoo.com/group/boost/files/pointer%20container/smart_containe... The package contains all you need to sucessfuly use the library with the Boost version 1.31 or later, however library tests require the latest Boost.Test, that is available from the main Boost CVS. What does the library provide? The library documentation states:

...

...

...

...

...

>>>>>>>>>>

Whenever a programmer wants to have a container of pointers to heap-allocated objects, there is usually only one exception-safe way: to make a container of smart pointers like boost::shared_ptr. This approach is suboptimal if 1. the stored objects are not shared, but owned exclusively, or 2. the overhead implied by smart pointers is inappropriate This library therefore provides standard-like containers that are for storing heap-allocated or cloned objects (or in case of a map, the mapped object must be a heap-allocated or cloned object). For each of the standard containers there is a smart container equivalent that takes ownership of the objects in an exception safe manner. In this respect the library is intended to solve the so-called polymorphic class problem. The advantages of smart containers are 1. Exception-safe and fool proof pointer storage and manipulation. 2. Notational convenience compared to the use of containers of pointers. 3. Can be used for types that are neither Assignable nor Copy Constructible. 4. No memory-overhead as containers of smart pointers can have (see 11 and 12). 5. Usually faster than using containers of smart pointers (see 11 and 12). 6. The interface is slightly changed towards the domain of pointers instead of relying on the normal value-based interface. For example, now it is possible for pop_back() to return the removed element. The disadvantages are 1. Less flexible than containers of smart pointers <<<<<<<<<<<<<<< As an example of how the new containers look like, consider that <code> typedef boost::shared_ptr<Foo> foo_ptr; std::vector<foo_ptr> vec; vec.push_back( foo_ptr( new Foo ) ); (*vec.begin())->bar(); </code> now becomes <code> boost::ptr_vector<Foo> vec; vec.push_back( new Foo ); vec.begin()->bar(); </code> In your reviews, please include the answers for the following questions: * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Also, in the documentation you will find questions from the submitter to the reviewer; whenever you feel you have something to contribute on these issues, please don't hessitate to include that in your review. Thank you, Pavol Droba, review manager for the Smart Containers library. ===8<===========End of original message text===========

On Thu, Oct 07, 2004 at 12:06:04PM +0200, Stefan Slapeta wrote:

Pavol Droba wrote:

...

Hi Boosters,

Formal review of the Smart Containers library from Thorsten Ottosen has been extended until Saturday, October 7th 2004). ^^^

Did you mean Saturday, October 9th? ;)

Oh, sure 9th is the correct date. Sorry for that. Pavol

Hi Jonathan, Thanks for your review. | Here's my review. I believe the Smart Container library should be ACCEPTED. I | have some suggested modifications, but there is only one issue which I think | needs to be resolved before I can give the library my unconditional approval | (item 5 under "evaluation of the design"). Since I did not have time to study | the implementation, my yes vote depends on other reviewers having done so and | found it acceptable. Thanks...am sure we can solve those issues. | > * What is your evaluation of the design? | | Very good, on the whole. I have these comments (in no particular order): | | 1. The functions used in the definition of Clonable are not well named. I would | prefer either | - allocate_clone/deallocate_clone or construct_clone/destroy_clone, to mimic | the interface of standard allocators, or | - something using new/delete, as someone else suggested yeah, there is some inconsistency here. | 2. It's a bit odd that CloneManager uses operator() to destroy objects. I | understand that this is a requirement of using CloneManager as a deleter, but | you could easily use an adapter instead. ok. | 3. I think Cloner sounds better than CloneManager. Cloner is already an English | word; CloneManager sound like a mixed metaphor. As I mentioned in another mail, and as sugeested by Pavol, it would probably be better to rip CloneManager apart into Allocator and CloneAllocator | 4. Since performance is one of the main design goals, I'm suprised that | attempting to insert a null pointer causes an exception rather than an insertion | failure. The same goes for invoking the functions for back, front, pop_back and | pop_front with an empty container. By coincidence, this morning I was | experimenting with some code that invokes a function pointer with a pointer as | argument, like so | | f_(p_) | | in an inner loop. I tried replacing the above code with | | if (p_) | f_(p_); | else | throw std::runtime_error(...); | | I found an enormous slowdown, which should be no surprise. yeah, I dunno, this might depend heavily on the compiler. what compiler do you use? At least for back()/front() you can avoid a check by *ptr_begin()/*--ptr_end(). For vector/deque you can unchecked random access too via operator[](). It is is true that performance was a design goal, but so was safety. The domain of pointers allowed me to do different trade-offs than with standard containers. For example, does it really matter if container.push_back( new T ) throw if 0 was inserted? IMO no, because the heap operation will dwarf any other operation by several magnitudes. The same considerations would be true for eg. pop_back() when calling the detructor on the object since we need to remove the object from the heap. | 5. The caveat "splice()/merge() in ptr_list is not thought through yet" makes me | very nervous. What aspect needs to be thought through? Does the current | implementation have well-defined semantics? I have not examined the | implementation, so I can't answer these questions, but whatever the caveat | means, resolving it should be a condition of acceptance. yes, I understand your concern and I promise to find a solution to this. It's just that each function needs to be faily carefully examined to see if its specs can potentially lead to leaks and other problems. I just didn't have the time, and I thought the review could work without people worriying too much about splice(). | 6. To answer possible review issue 13, I'd say you should parameterize the | provided clone managers by allocator type, defaulting to std::allocator< | [unspecified] >. why unspecified instead of T* ? | 7. I don't understand review issue 10. I don't see the erase()/insert() | functions that return pair<iterator, bool>. in set/map and ptr_set/ptr_map these functions returns this pair to say "we did/did not insert the element". I think that the same should hold for transfer; it would be only natural. Another way would of course to return optional<iterator>. | 9. For possible review issue 5, theoretcially I'd like to see auto_type exposed | as move_ptr. (I now like Howard Hinnant's suggested "unique_ptr", though he | seems to be leaning toward "sole_ptr" now.) Then you could also use move_ptr | where you now use auto_ptr. However, I agree with one of the other reviewers who | said that this would really be appropriate only if move_ptr were a stand-alone | library. I haven't proposed move_ptr mainly because I'm not sure whether it will | be made obsolete by policy_ptr and because while there was a great deal of | discussion in January, there wasn't much comment after I posted my recent | version. yeah, we'll wait and see. | 10. If ptr_iterators are kept, I agree that there needs to be a much more | detailed explanation of which algorithms they can be used with. ok. | > * What is your evaluation of the implementation? | | The parts I looked at looked good, but that means little since I only gave it a | quick glance. | | > * What is your evaluation of the documentation? | | It was hard to find my way around in the documentation. In general, there should | be a better index and more hyperlinks. My javascript menu is now a de facto part | of boost, so you might consider using it ;-) Here are some more detailed | comments, in no particular order: yes, good idea. [I have snipped most of your detailed comments, but I will look into them] | 6. On mozilla, some of the preformatted text overflows the bounding box, e.g., | in "3. Copy-semantics of smart containers" hm...yeah, I don't really know why it does that. | 10. British and American cows say "Moo". | | 11. Sound that pigs make, according to google: | | oink - 160,000 hits :-) | > * What is your evaluation of the potential usefulness of the library? | | Extremely useful. Thanks! br Thorsten

"Jonathan Turkanis" <technews@kangaroologic.com> wrote in message news:cjvg1n$r8s$1@sea.gmane.org... | | "Thorsten Ottosen" <nesotto@cs.auc.dk> wrote in message | news:cjvavc$hot$1@sea.gmane.org... | > Hi Jonathan, | | > | 4. Since performance is one of the main design goals, I'm suprised that | > | attempting to insert a null pointer causes an exception rather than an | > insertion | > | failure. The same goes for invoking the functions for back, front, pop_back | > and | > | pop_front with an empty container. By coincidence, this morning I was | > | experimenting with some code that invokes a function pointer with a pointer | > as | > | argument, like so | > | | > | f_(p_) | > | | > | in an inner loop. I tried replacing the above code with | > | | > | if (p_) | > | f_(p_); | > | else | > | throw std::runtime_error(...); | > | | > | I found an enormous slowdown, which should be no surprise. | > | > yeah, I dunno, this might depend heavily on the compiler. what compiler do you | > use? | | Good point. I was using VC7.1. I ran the same test with como4.3.3/VC7.1 Intel | 8.0 for windows and GCC 3.4.3 (Cygwin) and didn't see a significant difference. | But the fact that it makes a difference for some compilers, esp. a widely used | one, should be significant. There is one thing we often overlook when we want to place 0 into a large container: we must check every indexing for not being zero and that might counter-balance the effect of putting 0 in there in the first place. I did some testing with a handy null_object helper class that avoids all allocation overhead of null-objects. ( used like null_animal : public boost::null_object<animal,null_animal> { .. }; ) Then I compared code like for_each(.ptr_container_with_null_objects ) { i->foo(); // non-virtual i->bar(); // virtual } with for_each( standard_container_with_0s ) { if( *i ) { (*i)->foo(); // non-virtual (*i)->bar(); // virtual } } on vc7.1 In this setting a if the we have 2 : 5 nulls (2 nulls, 3 normal), checking for 0 wins. If we have 1 : 5 nulls or better the null object method wins. Basically I would guess this ratio depends entirely on how many if() we can execute for each virtual function dispatch since the body of null-object function is always trivial. Conclusion: disallowing 0 will not only lead to simpler, safer programs, but it will probably also boost performance. br Thorsten

"Pavol Droba" <droba@topmail.sk> wrote in message news:20041008185302.GN1510@lenin.felcer.sk... | Hi Thorsten, | | On Fri, Oct 08, 2004 at 08:11:28PM +0200, Thorsten Ottosen wrote: | > Then I compared code like | > | > for_each(.ptr_container_with_null_objects ) | > { | > i->foo(); // non-virtual | > i->bar(); // virtual | > } | > | > with | > | > for_each( standard_container_with_0s ) | > { | > if( *i ) | > { | > (*i)->foo(); // non-virtual | > (*i)->bar(); // virtual | > } | > } | > | > on vc7.1 | > | > In this setting a if the we have 2 : 5 nulls (2 nulls, 3 normal), checking for | > 0 wins. If we have 1 : 5 nulls or better | > the null object method wins. Basically I would guess this ratio depends | > entirely on how many if() we can execute for each virtual function | > dispatch since the body of null-object function is always trivial. | > | > Conclusion: disallowing 0 will not only lead to simpler, safer programs, but | > it will probably also boost performance. | | I'm sorry, but I must disagree with you analysis. You are looking to the problem | only from the smart containers perspective. This leads to fundamentaly wrong assumptions | and wrong results. | | Lets us think for a while what is the difference between null_object and 0 pointer. | | If we want to apply your semantics, null object must have fully defined interface. | Although its function will probably do nothing, still they must behave correctly. | So it is no longer a null_object. It is an object with a special state. I don't understand why you can't give it special state; IMO it is not necessary, though. | If you relax this requiremnt you will get to the exactly same scenario as with 0 | poiner, only with slightly bigger overhead. Why? Becase if I designate something | as null, I will have to check for it anyway. So your example will look like this: | | for_each(.ptr_container_with_null_objects ) | { | if(!i->is_null) | { | i->foo(); // non-virtual | i->bar(); // virtual | } | } do you mean a dynamic cast? or how do ->is_null look like? | Otherwise, you will have pay the comparison cost every time you perform an operation | on the null object, and that might be on far more places. not understood. | In other words. If I designate an object as null, I want to threat it as an invalid | null object. Therefor I will have to check every operation I would with 0 pointer. that's not what null objects are for. | Also, if the design of my application needs to put only non-null objects into the container, | I will NOT HAVE to check anything. true, but you have lost the guarantee. Thus any interface with ptr_container foo(); bar( const ptr_container& r ); you have to document explicitly whether you allow null or not. | At most I will put asserts there, but definitely not | exceptions. by "there" you mean where? | So your arguments are quite misleading. If you allow 0 pointers, it is still possible | to uses null_object pattern without any additional cost. The oposite is obviosly not | feasible. I need some real code to work with to be able to see this. Then I can rewrite it to compare. br Thorsten

"Pavol Droba" <droba@topmail.sk> wrote in message news:20041008202443.GA22250@lenin.felcer.sk... | > that's not what null objects are for. | | So what is the difference between your null-object and 0 pointer? | I see none, except in your case I will have to define special class/state | for every type I would like to use the smart_container with. the point is basically to avoid all if( !null ) logic. | > | > > Otherwise, you will have pay the comparison cost every time you perform an | > > operation | > > on the null object, and that might be on far more places. | > > | > not understood. | | Try to ask Jeff Garland, how many complications it brought when a date class in date_time | library was enriched with the special values. Every operation must be aware of it. | yes, the 0 pointer is a special value and we want to avoid the complications it brings. | > I need some real code to work with to be able to see this. Then I can rewrite | > it to compare. | > | | // Some classes to work with | class Base | { | virtual int Op1() =0; | ... | virutal int Op100() =0; | }; | | | class A : public Base | { | virtual int Op1() { // do something reasonable and return a valid value // } | .. | virtual int Op100() { // do something reasonable and return a valid value // } | }; | | | class Null : public Base | { | virtual int Op1() { return special_value1; } | .. | virtual int Op100() { return special_value100; } | }; I would return 0 here, in all cases. | // Now some operations, that use the object of Base. | // Assume we have 0 pointers | | int foo1(Base* pObj) | { | // Simple case -> check if oObj is not null | if(pObj==0) | return special_value; | else | return pObj->Op1() + pObj()->Op2() + ...... + pObj()->Op100(); | } int foo1( Base& pObj ) { return pObj->op1() + ... + pObj->op100(); } | This example might look realy stupid, but it illustrates well the problem I'm refering to. | You cannot assume, that null object will work in the same way as an ordinary one. Otherwise | you only pospone the checking one level further -> to its operations and the objects, that | uses it. Yeah, that is really the issue here: are we just forcing users to defer the comparinson for "nullness" yet another level. Am eager to be proven wrong, but It's going to take more convincing examples. I would like to see real examples and then see if I can recode them so we can compare. But it has to be real examples (you must have plenty from your work? Just take one where you use shared_ptr). But seriously, I don't recall the need for 0's for a long time. A long with that code I need to see why there is a need to put a 0 in the container in the first place and it would also be nice to know the density of them. br Thorsten

"Pavol Droba" <droba@topmail.sk> wrote in message news:20041008213049.GP1510@lenin.felcer.sk... | > I would like to see real examples and then see if I can recode them so we can | > compare. But it has | > to be real examples (you must have plenty from your work? Just take one where | > you use shared_ptr). | > But seriously, I don't recall the need for 0's for a long time. | | Not plenty, but indeed I have the cases. IIRC I have already described | one example. | | I'm using a container of smart pointer of a fixed size. Each place is designated | as a slot and althouch all members of container have common predecessor and functionality, | each slot has a special meaning. | Generaly, the container is used from more then entity. For one it is just a bunch | of objects that are used in an order, for another entity, each slot have a different meaning. | There are even assertions for a specific types, that have be present on a specific slot. | | Slot can be empty of cause. In that case, first entity simply skips it, second entity | is also aware of it an acts accordingly. | | If I would go your way, I will need a special class for every slot, because from the | POV of the second entity, they are all different. | | Do you need a code to see this? yes please. -Thorsten

Howard Hinnant wrote:

On Oct 5, 2004, at 6:21 PM, Jonathan Turkanis wrote:

...

(I now like Howard Hinnant's suggested "unique_ptr", though he seems to be leaning toward "sole_ptr" now.)

I am sooooooooo fickle! :-)

FWIW, sole_ptr doesn't look very appealing to this non-native speaker. ;-) unique_ptr isn't much better but at least I can associate it with shared_ptr::unique().

"Caleb Epstein" <caleb.epstein@gmail.com> wrote in message news:989aceac04100607466d2aa109@mail.gmail.com... | On Wed, 6 Oct 2004 13:36:17 +0300, Peter Dimov <pdimov@mmltd.net> wrote: | > Howard Hinnant wrote: | > > On Oct 5, 2004, at 6:21 PM, Jonathan Turkanis wrote: | > > | > >> (I now like Howard Hinnant's suggested "unique_ptr", though he | > >> seems to be leaning toward "sole_ptr" now.) | > > | > > I am sooooooooo fickle! :-) | > | > FWIW, sole_ptr doesn't look very appealing to this non-native speaker. ;-) | > | > unique_ptr isn't much better but at least I can associate it with | > shared_ptr::unique(). | | Perhaps lone_ptr? what was wrong with move_ptr? The fact that there is only one owner does not need to be reflected in the name; many pointers are like that...scoped_ptr, auto_ptr. br Thorsten