Re: [boost] [Review] RangeEx review extended to the 10th of March

Subject: Re: [boost] [Review] RangeEx review extended to the 10th of March
From: Rogier van Dalen (rogiervd_at_[hidden])
Date: 2009-03-10 20:20:26

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