Here's my review of the RangeEx library. I hope it's still on time. I'd first like to thank Neil for bringing forward this library. On Tue, 2009-03-03 at 20:53 +0100, Thorsten Ottosen wrote:
- What is your evaluation of the design?
Overall, the design is good. I think representing a sequence by one range is more user-friendly than using two iterators. Operations on sequences become even more user-friendly with RangeEx. Having written similar facilities before, I am looking forward to using RangeEx. I do have a couple of comments though: one I really think ought to be fixed (and I think the author agrees with me here so that shouldn't be a problem); and a few that have been discussed on the list and Neil will probably have to make a decision on. What I really think ought to be fixed is the algorithms that use output iterators (except for "copy"). Output iterators I think are awkward. Their use in the standard library seems to be to return a range. The obvious way to do this in RangeEx is to actually return a range, a lazy one. If a user wants to write, say, a transformed range to an output iterator, copy (transformed (rng, func), output_it); will do the job. I think this spelling more clearly separates the two actions (transforming and writing out) than an output iterator-based transform(). For performance, this should probably forward to std::transform(). IMO The output iterator versions of other operations and generators should also be replaced with lazy range versions. I think the author will have to consider these issues and the discussion on the mailing list: - operator| vs. function-style lazy ranges. I think function-style should be the default, both because it's well-known and because it generalises to multiple parameters. - The naming scheme for the three forms of operations. I think that make_*_range is too verbose and doesn't express the "lazy view" aspect. In my opinion, the names could express the operation, and not whether the mutating or lazy version is used. (Overloading or namespaces would help.) I think that would optimise both reading and writing of code. - I have no feel for the return value policies. Apparently people feel the need for this. I would only like to say, though, that the natural spelling for boost::erase( vec, boost::unique<boost::return_next_end>(vec) ); seems to be boost::unique(vec); but I am glad to leave decisions about this to Neil. - I think it would make sense to add a "zip" operation to the library. I don't think that "for_each over two ranges" should be a separate algorithm.
- What is your evaluation of the implementation?
I haven't spent a lot of time looking at the implementation, but it seems solid and clear, including the tests.
- What is your evaluation of the documentation?
I think the documentation is clear. I think someone mentioned the lack of examples for each of the algorithms, but I don't find that to be a problem myself. The one thing that might need an overhaul is the motivation for operator| which currently seems to depend on the verbosity of the make_*_range syntax. I think Dave Abrahams gave a more convincing argument.
- What is your evaluation of the potential usefulness of the library?
Very. I have missed it in every program over 50 lines I've written in C ++.
- Did you try to use the library? With what compiler? Did you have any problems?
I have, for a moment. gcc 4.3.2 on Linux. No problems.
- How much effort did you put into your evaluation? A glance? A quick - reading? In-depth study?
I looked at the documentation in quite a lot of detail. The implementation, however, I've given just a glance.
- Are you knowledgeable about the problem domain?
Anyone who's used Boost.Range must be. I have, however, written things similar to parts of this library before, so I assume I'm fairly aware of the problems and the possible solutions, yes.
And finally, every review should answer this question:
- Do you think the library should be accepted as a Boost library?
Yes. Cheers, Rogier