Re: [review] Formal review of the Smart Container library starts today]
Hi, First of all, I'd like to thank Thorsten for bringing up this library. It implements a very common programming pattern that is currently missing in STL or Boost.
* What is your evaluation of the design?
I find the design sufficient and reasonable for the purpose of the library. There is only one important issue, that I find problematic (I have already mentioned it in the other mail). The issue is about the direct write access to the contained pointers, via the ptr_iterator. Using this interface, all invariants of the underlying container can be broken. It is not even possible to validate the problem. Therefor I suggest to remove it, unless it can be fixed. Still this access can be usefull from time to time, but I simply don't think, that it is worth the problems, it can bring. Documentation is not very clear about the problems. It merely notes, that algorithms like std::remove should not be used with the ptr_iterator. There is no explanation which algorithms are 'safe' and why are the others unsafe. After small analysis I have found out that there is actualy a very narrow set of algorithms that can be used (I have explained this in detail in the other mail). All these algorithms can be provided as member functions (similary like current remove and unique algorithms) or can be reimplemented with a help of swap_element(size_type, size_type) function (or something similar). Other small issues: * CloneManager::operator(). I don't like this notation. I would prefer something more explicit. I see no clear connection between this operator and the operation it provides. Also I don't understand the asymetry between CloneManager::clone function and operator(). Former is static, while the later is member function. Why? * I would like to see explicit allocator specification for underlying container. There were good reasons to provide it in the STL and from the same reasons I'd like to see it here. * Current implementation is restricted to non-null values. Still I think, that null support could be useful. Few functions for checking like is_null(index) and iterator.is_null() could handle do job.
* What is your evaluation of the implementation?
I find the implementation good enough to be included in the boost.
* What is your evaluation of the documentation?
Documentation is sufficient for the understanding the library, but there are several posibilities for improvements. First, I agree with point that was already mentioned here, that several small examples are better a long one. Introductory example is too long to be easily comprehended in the first reading. There is lot of rules and guideling that are explained only in a few sentences. Some of them would greatly benefit from a small example and others (like the one about 'safe' algorithms) are very underspecified. I would presonaly prefer several smaller hyperlinked files to a one long document. Although it is easier to print it, I found it harder to read it in the browser.
* What is your evaluation of the potential usefulness of the library?
I think, that the library is very usefull. As I said it implements very common pattern, that is not sufficiently solved otherwise.
* Did you try to use the library? With what compiler? Did you have any problems?
I have not compiled the library.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Few hours of reading the documentation and code and thinking about the possible issues. Also I spend some time in discussion with the author.
* Are you knowledgeable about the problem domain?
This library deals with a common problem. I consider myself knowledgeable in this domain.
* 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.
Yes, I do think that the library should be ACCEPTED as a Boost library. However I would like to see the issues I have described to be solved, or at least explained in the documentation in the sufficient detail. Here are my answers for some questions asked in the documentation:
1. The reference type, SmartContainer::reference is indirected. This has the unfortunate consequence that eg. we cannot use eg. ptr_vector with std::stack. The question is if its worth adding another policy just to achieve this.
Considering the library purpose, I think, that the current state is sufficient.
2. Is the ptr_array class worth the trouble?
I would like to see it. Especialy if the null-value can be supported.
3. The clone manager effective removes allocators from the container template parameter interface. Can people who want to use allocators still do what they want?
Yes, I'd like to see it there.
5. Should SmartContainer::auto_type expose itself as a move_ptr?
Unless the move_ptr would became a self-contained utility, possibly as another library, I think it should stay like this.
6. Should iterator range versions of assign() and insert be replaced by an Boost.Range version? Ie, should we have
template< class SinglePassRange > void insert( const Range& r );
instead of
template< class InputIterator > void insert( InputIterator f, InputIterator l );
Boost.Range is already a part of boost. I think that the later interface is obsolete now.
10. should transfer() return iterator or iterator/bool pair like erase()/insert() do.
For map, transfer has similar semantics to insert. I think, that interfaces should be similar as well.
12. should we provide member algorithms for unsafe standard algorithms like unique() and remove?
Eithter provide them as member function, or provide a means for standalone implementation. In any case it is wise to provide some functional variant. Best Regards, Pavol
Hi Pavol, Thanks for your review. "Pavol Droba" <droba@topmail.sk> wrote in message news:20041003224329.GV9207@lenin.felcer.sk... | Hi, | | First of all, I'd like to thank Thorsten for bringing up this library. | It implements a very common programming pattern that is currently | missing in STL or Boost. you're welcome. | > * What is your evaluation of the design? | | I find the design sufficient and reasonable for the purpose of the library. | | There is only one important issue, that I find problematic (I have already mentioned it in | the other mail). | | The issue is about the direct write access to the contained pointers, via the ptr_iterator. | Using this interface, all invariants of the underlying container can be broken. It is not | even possible to validate the problem. Therefor I suggest to remove it, unless it can | be fixed. | | Still this access can be usefull from time to time, but I simply don't think, that it | is worth the problems, it can bring. ok, noted, but I dont agree. | Documentation is not very clear about the problems. It merely notes, that algorithms | like std::remove should not be used with the ptr_iterator. There is no explanation which | algorithms are 'safe' and why are the others unsafe. Did you see the FAQ entry "Which mutating algorithms are safe to use with pointers?"? I could of course go through every mutating algorithm in the standard library, but then we would still miss all other algorithms in boost or custom made. | After small analysis I have found out that there is actualy a very narrow set of | algorithms that can be used (I have explained this in detail in the other mail). | All these algorithms can be provided as member functions (similary like current | remove and unique algorithms) or can be reimplemented with a help of swap_element(size_type, size_type) | function (or something similar). the problem with this is AFAICT scalability. which algorithms do we consider essential and what guarantee do we have that others do too? | Other small issues: | | * CloneManager::operator(). | | I don't like this notation. I would prefer something more explicit. I see no clear connection | between this operator and the operation it provides. | | Also I don't understand the asymetry between CloneManager::clone function and operator(). | Former is static, while the later is member function. Why? First, operator()() cannot be static. Second, operator()() is provided because a clone manager act as a deleter passed to the internal move ptr and other classes. | * I would like to see explicit allocator specification for underlying container. There were good | reasons to provide it in the STL and from the same reasons I'd like to see it here. I actually agree; the clone manager should be broken up into the original Allocator and a CloneAllocator policy having ::clone() and operator()() members. | * Current implementation is restricted to non-null values. Still I think, that null support | could be useful. don't you like the null object pattern? | Few functions for checking like is_null(index) and iterator.is_null() could handle do job. I think the core working group is going to allow null references, which means if( &*begin() ) becomes legal. See also active issue "232. Is indirection through a null pointer undefined behavior? ". From the latest discussion: " Notes from the October 2003 meeting: See also issue 315, which deals with the call of a static member function through a null pointer. We agreed that the approach in the standard seems okay: p = 0; *p; is not inherently an error. An lvalue-to-rvalue conversion would give it undefined behavior " However, it is true that one reason for the containers to not allow 0 is to avoid troubles with 0 indirection on iterators. | > * What is your evaluation of the implementation? | | I find the implementation good enough to be included in the boost. | | > * What is your evaluation of the documentation? | | Documentation is sufficient for the understanding the library, but there are several posibilities | for improvements. indeed :-) | First, I agree with point that was already mentioned here, that several small examples | are better a long one. Introductory example is too long to be easily comprehended in the | first reading. | | There is lot of rules and guideling that are explained only in a few sentences. Some of them | would greatly benefit from a small example and others (like the one about 'safe' algorithms) | are very underspecified. ok. | I would presonaly prefer several smaller hyperlinked files to a one long document. Although | it is easier to print it, I found it harder to read it in the browser. yeah, some is already hyperlinked, but I guess more can be put into seperate files. | > * 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. | | Yes, I do think that the library should be ACCEPTED as a Boost library. However | I would like to see the issues I have described to be solved, or at least | explained in the documentation in the sufficient detail. Thanks! | > 2. Is the ptr_array class worth the trouble? | | I would like to see it. Especialy if the null-value can be supported. that is what makes ptr_array a bit different; it needs to make its constructed state 0; I'm not convinced that allowing 0's is any benefit in general, but I'm open to is_null( index ) and is_null( iterator ) predicates. br Thorsten
"Thorsten Ottosen" <nesotto@cs.auc.dk> wrote in message news:cjq68v$2kv$1@sea.gmane.org... | "Pavol Droba" <droba@topmail.sk> wrote in message | | Other small issues: | | | | * CloneManager::operator(). | | | | I don't like this notation. I would prefer something more explicit. I see | no clear connection | | between this operator and the operation it provides. | | | | Also I don't understand the asymetry between CloneManager::clone function | and operator(). | | Former is static, while the later is member function. Why? | | First, operator()() cannot be static. | | Second, operator()() is provided because a clone manager act as a deleter | passed to the internal move ptr | and other classes. thinking about this makes me realize that it is probably wrong to specify ::clone() as static; in the case of a stateful CloneAllocator, we probably don't want that. -Thorsten
On Mon, Oct 04, 2004 at 02:41:07AM +0200, Thorsten Ottosen wrote:
Hi Pavol,
Thanks for your review.
"Pavol Droba" <droba@topmail.sk> wrote in message news:20041003224329.GV9207@lenin.felcer.sk...
[snip]
| Documentation is not very clear about the problems. It merely notes, that | algorithms | like std::remove should not be used with the ptr_iterator. There is no | explanation which | algorithms are 'safe' and why are the others unsafe.
Did you see the FAQ entry "Which mutating algorithms are safe to use with pointers?"?
I did see it and I find it unsufficient. I does provide the information, but it can be easily overlooked. Such an information should go to some more important section and longer explanation should be gievn
I could of course go through every mutating algorithm in the standard library, but then we would still miss all other algorithms in boost or custom made.
| After small analysis I have found out that there is actualy a very narrow | set of | algorithms that can be used (I have explained this in detail in the other | mail). | All these algorithms can be provided as member functions (similary like | current | remove and unique algorithms) or can be reimplemented with a help of | swap_element(size_type, size_type) | function (or something similar).
the problem with this is AFAICT scalability. which algorithms do we consider essential and what guarantee do we have that others do too?
As I already stated std algorithms should be provided along with the means to implement the user-specific ones.
| Other small issues: | | * CloneManager::operator(). | | I don't like this notation. I would prefer something more explicit. I see | no clear connection | between this operator and the operation it provides. | | Also I don't understand the asymetry between CloneManager::clone function | and operator(). | Former is static, while the later is member function. Why?
First, operator()() cannot be static.
Second, operator()() is provided because a clone manager act as a deleter passed to the internal move ptr and other classes.
This is an internal issue and I see no reason why should it affect the user-level interface.
| * I would like to see explicit allocator specification for underlying | container. There were good | reasons to provide it in the STL and from the same reasons I'd like to see | it here.
I actually agree; the clone manager should be broken up into the original Allocator and a CloneAllocator policy having ::clone() and operator()() members.
| * Current implementation is restricted to non-null values. Still I think, | that null support | could be useful.
don't you like the null object pattern?
This seems like a workaround for a functionality that can be naturaly provided otherwise. I'd prefer null pointer. [snip]
However, it is true that one reason for the containers to not allow 0 is to avoid troubles with 0 indirection on iterators.
I think, you are little be contradictive. One place you allow unsave manipulation with the internal data one the other place you are cautious about the indirection problems, that can be easily checked. [snip]
| > 2. Is the ptr_array class worth the trouble? | | I would like to see it. Especialy if the null-value can be supported.
that is what makes ptr_array a bit different; it needs to make its constructed state 0; I'm not convinced that allowing 0's is any benefit in general, but I'm open to is_null( index ) and is_null( iterator ) predicates.
I'm using arrays of smart-pointers quite often. A specific use case is a slot structure. An array is a holder for slots. Each slot is designated by a constant (index) and can hold a pointer to an object. It can also be empty. There is predefined number of slots, each with a different functionality. Imagine a set of transformations, for an image. This set can consist of - Bit-Depth adjustment - Scale - Distortion - ... Some of these slots can be occupied, others can be empty. I need precise indexing, so these transforms are easily accessible from other components (like gui). Once a transform is set, array takes the ownership of it and deletes it upon the exit. Regards, Pavol
"Pavol Droba" <droba@topmail.sk> wrote in message news:20041004100730.GY9207@lenin.felcer.sk... | On Mon, Oct 04, 2004 at 02:41:07AM +0200, Thorsten Ottosen wrote: | > However, it is true that one reason for the containers to not allow 0 is to | > avoid troubles with 0 indirection on iterators. | | I think, you are little be contradictive. One place you allow unsave manipulation with | the internal data one the other place you are cautious about the indirection | problems, that can be easily checked. easily? The whole interface is indirected. Doing if( is_null(...) ) in front of every member function call is not going to help anybody. What is most contradictive: 1. to prefer safety in general, but to allow unsafe operations through ptr_iterator 2. to prefer safety of ptr_iterator, but to allow unsafe operations in general ? br Thorsten
On Mon, Oct 04, 2004 at 03:46:18PM +0200, Thorsten Ottosen wrote:
"Pavol Droba" <droba@topmail.sk> wrote in message news:20041004100730.GY9207@lenin.felcer.sk... | On Mon, Oct 04, 2004 at 02:41:07AM +0200, Thorsten Ottosen wrote:
| > However, it is true that one reason for the containers to not allow 0 is to | > avoid troubles with 0 indirection on iterators. | | I think, you are little be contradictive. One place you allow unsave manipulation with | the internal data one the other place you are cautious about the indirection | problems, that can be easily checked.
easily? The whole interface is indirected. Doing if( is_null(...) ) in front of every member function call is not going to help anybody.
If you put 0 in the container, you are aware of it, because you have put it there. Therefore you can act accordingly. It is easy to put asserts in the dereferencing member functions to guarantie to spot errors. If you don't use 0 pointers, than you don't have to worry, since the semantics of smart container does not allow to "create" a 0 pointer.
What is most contradictive:
1. to prefer safety in general, but to allow unsafe operations through ptr_iterator 2. to prefer safety of ptr_iterator, but to allow unsafe operations in general
The difference I'm trying to point out, is that using of 0 pointers can be well specified and checked in the runtime. It is possible to adjust container invariants to comprehend this. Even if I make a mistake, access violation is generated exactly on the place where error happens. I consider this behaviour on the same safeness-level as all STL structures. On the other hand, ptr_iterator directly exposes the internal structure of the container. It is similar as if shared_ptr would allow the access to its private px member. I consider this to be quite big design flaw. It goes againt rules of good design. Also faulty behaviour is harder to spot and debug. Regards, Pavol
"Pavol Droba" <droba@topmail.sk> wrote in message news:20041004143908.GA9207@lenin.felcer.sk... | On Mon, Oct 04, 2004 at 03:46:18PM +0200, Thorsten Ottosen wrote: | > "Pavol Droba" <droba@topmail.sk> wrote in message | > news:20041004100730.GY9207@lenin.felcer.sk... | > | On Mon, Oct 04, 2004 at 02:41:07AM +0200, Thorsten Ottosen wrote: | > | > | > However, it is true that one reason for the containers to not allow 0 is | > to | > | > avoid troubles with 0 indirection on iterators. | > | | > | I think, you are little be contradictive. One place you allow unsave | > manipulation with | > | the internal data one the other place you are cautious about the indirection | > | problems, that can be easily checked. | > | > easily? The whole interface is indirected. Doing if( is_null(...) ) in front | > of every | > member function call is not going to help anybody. | > | | If you put 0 in the container, you are aware of it, because you have put it there. | Therefore you can act accordingly. you might be able to, but other users can't see from the declaration of ptr_vector<T> that there is 0 in there. | It is easy to put asserts in the dereferencing | member functions to guarantie to spot errors. these go away in release mode and won't catch code not found during testing. it might be an idea to have bool CloneAllocator::allow_null or a designated NullPolicy? | The difference I'm trying to point out, is that using of 0 pointers can be | well specified and checked in the runtime. It is possible to adjust container | invariants to comprehend this. | Even if I make a mistake, access violation is generated exactly on the place | where error happens. on what platforms; on what compilers? you have undefined bahavior and anything might happen. and if your test don't run all code or all path through the code, you might get this behavior after you dispatched your application. | I consider this behaviour on the same safeness-level as all STL structures. | | | On the other hand, ptr_iterator directly exposes the internal structure | of the container. It is similar as if shared_ptr would allow the access to | its private px member. | I consider this to be quite big design flaw. It goes againt rules of good design. | Also faulty behaviour is harder to spot and debug. IMO there is a big difference between allowing *all* interface functions to lead to undefined behavior and allowing two functions to be potentially misused. I simply don't understand what you've got against the null object pattern: - you avoid undefined behavior - your code logic is simplified what's there to loose? br Thorsten
participants (2)
-
Pavol Droba -
Thorsten Ottosen