Re: [boost] Review: Iterator adaptors

From: David Abrahams (abrahams_at_[hidden])
Date: 2000-12-28 15:11:20

I'm finally catching up with review comments here...
----- Original Message -----
From: "Beman Dawes" <beman_at_[hidden]>
To: <boost_at_[hidden]>
Sent: Monday, December 18, 2000 8:39 AM
Subject: Re: [boost] Review: Iterator adaptors

> I think the iterator adaptors library should be accepted by boost.
>
> Issues
> ------
>
> * Class names - class names which differ only by being singular or plural:
>
>       struct iterator_adaptor;
>       struct iterator_adaptors;
>
> This can cause endless confusion.  In verbal communication, the two are
> hard to distinguish.  In written communication, it is always hard to tell
> if you are looking at a typo.  Real typos become serious because they
> change meaning. It gets worse if English is a second language for those
> involved.
>
> So please consider a name change.  Perhaps:
>
>       struct iterator_adaptor;
>       struct dual_iterator_adaptor;

How about iterator_adaptor and iterator_generator?
I almost think that iterator_adaptor ought to be called simply "iterator".
As John Potter has pointed out recently, it can turn almost any piece of
data into an iterator... but that would be a bad name choice.

> * Header names - if a boost header file is intended for eventual inclusion
> in an existing C++ standard library header, should boost use the same
> name?  For example, should boost/iterator_adaptors.hpp be
> boost/iterator.hpp?

I don't think that's neccessarily helpful. Right now we have boost headers
(such as iterator.hpp) which provide missing standard functionality for
non-conforming compilers. It's probably presumptuous to try to anticipate
the eventual header structure of the standard, especially as the standard
library grows.

> * Header names - singular or plural?  The C++ standard library header
names
> are all singular, except for <limits>.  Should boost follow that practice?

I don't care much. If I change iterator_adaptors to iterator_generator, I'd
change iterator_adaptors.hpp to iterator_adaptor.hpp, though.

> * Issue to work out with Jens Maurer:  Conceptually integer_range and
> generator_iterator are similar.  Are they similar enough so that the names
> should be coordinated?  Or could one even subsume the other? (Jens: it
> would help if http://www.boost.org/libs/random/random-misc.html had usage
> examples.)

Whoops, that may make iterator_generator a confusing name for me to use. Is
generator_iterator actually more similar to transform_iterator? We need to
work this out.

I note the following comment in the generator_iterator implementation:
  // Note: deriving from std::iterator<> is useless
Au contraire, my fine friend! If you derive from std::iterator your iterator
stands a chance of working with algorithms on a compiler without partial
specialization. With STLport under MSVC, for example, a user-defined
iterator won't work right... unless it is derived from std::iterator<>.

> * The transform_iterator_policies() default constructor looks odd.  Is it
> correct?  It looks like a safety issue - a default constructed
> transform_iterator_policies instance will be leave m_f invalid.

I think that's OK. Since m_f is an AdaptableUnaryFunction, it can't be a raw
pointer-to-function. Did Jeremy already answer this?

> It also
> seems a bit odd that transform_iterator_policies is a struct rather than a
> class - I must be missing something.

Well, I think you're right at least that the data member should be private.
Comments, Jeremy?

> * reverse_iterator/reverse_iterators is also likely to cause great
> confusion.

I think reverse_iterators->reverse_iterator_generator is probably the right
answer here.

> Documentation Issues
> --------------------
>
> * The iterator_range example is a strong motivator:
>
>       boost::integer_range<int> r(0,5);
>
>       cout << "counting to from 0 to 4:" << endl;
>       std::copy(r.begin(), r.end(), ostream_iterator<int>(cout, " "));
>       cout << endl;
>
>    Perhaps it could be moved to the introduction to suck people in. But
> don't move the explanation of how it works; make readers persevere to
learn
> that.

I'll consider that.

> * As I was reading the Iterator Adaptors Class docs, I kept wondering how
> they would work for containers where iterator and const_iterator were the
> same type.  Of course I found out when I got to the Iterator Adaptor Class
> docs, but an earlier mention would have been appreciated.
>
> * Boost also includes iterator helpers in boost/operators.hpp.  There
> should be some cross-referencing between the docs so if someone
> inadvertently looks at the wrong one for their need they will be directed
> to the other.
>
> * Iterator Adaptors Class docs, paragraph 3, that begins "The Policies
> class...":
>
>      -  Style guides usually specify numbers below 10 should be spelled
> out.

OK

>      -  The term "core iterator operations" is used without being defined.

OK

>      -  "Make sure that the iterator_category type of the traits class you
> pass in matches..." would be clearer if it read "The iterator_category
type
> of the traits class you pass in must match..."

OK

> * Challenge:  "There is an unlimited number of ways the the ..." should be
> "There are an unlimited number of ways the..."

OK

I checked those changes into CVS.