Re: [boost] Review: Iterator adaptors

From: Beman Dawes (beman_at_[hidden])
Date: 2000-12-18 08:39:18

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;

* 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?

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

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

* 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.  It also 
seems a bit odd that transform_iterator_policies is a struct rather than a 
class - I must be missing something. If the default constructor is correct, 
a comment as to the rationale might be useful.  Ditto for the 
projection_iterator_policies.

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

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.

* 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 type 
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..."

All in all, a nice effort!

--Beman