Dear all, It is my pleasure that Neil Groves' RangeEx library has been accepted into boost. Congratulations Neil! There are quite a number of minor issues that need to be resolved before the library is release ready, see below for a summary. Review statistics ----------------- Full Reviews: 8. Discussion: extensive. I had the clear impression that everybody that participated in the discussion were in favor of this library, albeit they did not have time to submit a full review. I did not hear a single statement saying that this library should be rejected. Thanks to everybody that participated in the review and its discussions. Issue Summary ------------- Below is given a list of topics that must be adressed before the library can be included into boost. In general, we should try to discuss them one at a time in seperate threads. Many people suggested various extensions, new algorithms (e.g. from adope), etc. **In general we should not require Neil to add all these if he does not have the time currently: the basic infrastructure, the concepts, the naming and organization of the library is much more important. Then we can add all these things later.** 1: documentaion =============== The documentation should clearly reflect if an algorithm delegates to a standard algorithm. If the algorithm is new, then it should state complety and exception-safety. More examples would be welcome. Sometimes the rationale and explanations could be improved, e.g for operator|(). 2: return type specification for find() etc =========================================== There where no major objection to the mechanism, but some found the syntax ugly. I believe the suggested syntax (e.g.) boost::find[_f,_e]( range, x ) boost::find[_f+1,_e]( range, x ) was considered good by most. 3: namespace organization ========================= Currently the library uses the namspaces boost boost::adaptors Some raised concern about putting the algorithms straight in the boost namespace before we have a general agreement on where to put algorithms (e.g. in boost::algorithm or boost::ranges or whatever). I think it will make sense to put each algorithm in its own header, since we might have to include additional standard library headers for calling algorithms that are implemented as member functions (but see 7 below). Feedback is most welcome. 4: naming convention for adaptors ================================= The following have been proposed throughout the review (here examplified with "transform"): | transformed( fun ) | transform( fun ) | transform_view( fun ) | view::transform( fun ) There where no consensus during the review. Other libraries seem to use the _view suffix, which is a strong argument for that candidate (in that case, I suggest to drop the adaptors namespace). 5: naming convention for the adaptor generators =============================================== Many people like to be able to say make_transform_range(r,fun) instead of r | transformed( fun ). There was no concensus on how to name these functions. Here are some candidates: make_transform_range( r, fun ) transformed( r, fun ) transform( r, fun ) (*) transform_view( r, fun ) (*) was dislike by some because it has exactly the same spelling as the algorithm transform. Many felt that the confusion is too big if view generators are not named different from the actual algorithms. 6: reintroducing _if variants of algorithms =========================================== An important problem with the suggested approach was the the iterator returned by an algorithm after applying | filtered( pref ) are now filtered iterators and so one needs to manually unwrap the return iterator. This provided enough justification for reintroducing these algorithms. (One could imagine that this unwrapping was done automatically by a conversion operator such that iterator i = boost::find( r | boost::filtered( pred ), x ); "just worked". However, the syntax is still slightly worse than just using the original algorithm). Generic programming is concerned with the use of orthogonal concepts which put together yields all possible combinations of algorithms. If we reintroduce all _if algorithms, we have to aks ourselves "when does it end?". Should all new algorithms then simply add _if variants? This seems very much against the spirit of generic programming. The problem with the iterators being changed (as in the find example) seems to suggest the following guideline: "If the algorithm returns an iterator, it makes sense to provide an _if overload. Otherwise it does not." Under this guideline, find_if() should be there, but count_if() should not be provided. 7: should algorithms call member function when possible? ======================================================== I heard one strong voice against this. The reason was that the algorithm implemented as member function often has quite different guarantees w.r.t. complexity, reference and iterator stabililty. I think this is a very strong argument. Also, we can add this later if good arguments appear, but it is much harder to take away. We also have to remember Scott Meyers item "Beware of container independent code" which also suggest that this is a bad idea. At the very menimum, the original algorithm and the member function should have similar semantics. This seems to suggest that we can call set::lower_bound() from boost::lower_bound(). In that case, the best way would probably be to add these as overloads in the boost namespace: template< class T, class A > typename std::set<T,A>::iterator lower_bound( std::set<T,A>& s ); template< class T, class A > typename std::set<T,A>::const_iterator lower_bound( const std::set<T,A>& s ); 8: making return values of algorithms consistent/intuitive ========================================================== For example, sort(r) returns the sorted range, but sort_heap(r) does not. Similar issues for partial_sort(). Please go through all algorithms to see if this done correctly. 9: output range concept? ========================= It appears that the only use for an output iterator in the library was for boost::copy(rng,iter), and the only use for that function was for "sinks" like std::ostream_iterator<T>(...). This seems to be the only safety hole left in the library. Here's an idea to how we might remove that: create a new "copy sink" concept: struct array_copy_sink { ... template< class Iterator > copy( Iterator begin, Iterator end ) { ... } }; Then we might imagine boost::copy( rng, boost::ostream_sink(std::cout) ); boost::copy( rng, boost::array_sink(an_array) ); boost::copy( rng, boost::ptr_sink(begin,end) ); boost::copy( rng, boost::front_insert_sink(a_deque) ); ... For optimal performance, we also need a "nonoverlapping copy sink" concept, and an algorithm that expects that sink (or that boost::copy() determines the type of the sink automatically). 10: automatic projection ======================= Adope has algorithms overloaded such that projection is very easy: struct my_type { int member; double another_member; }; //... my_type a[] = { { 1, 2.5 }, ... }; // sort the array by the first member: sort(a, less(), &my_type::member); my_type* i = lower_bound(a, 5, less(), &my_type::member); We should be able to express this well, albeit not as concise, with my_type* i = boost::lower_bound( a | project( &my_type::member ), 5, less() ); where project( ... ) could simply return a transform iterator constructed with boost::bind(). There is another problem, however, because now the return value is a tranform_iterator of some form. Appending .base() would solve it, albeit it is somewhat ugly. Again this might raise the question of an implicit conversion. I just want to say this is not a critical issue as we can always add these overloads. --- End of Issues ---- Again, I would like to thank everybody that participated in the review. best regards Thorsten, review manager