Boost logo

Boost :

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
> 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 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
> 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.


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


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


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


I checked those changes into CVS.

Boost list run by bdawes at, gregod at, cpdaniel at, john at