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