Iterator class for vector and basic_string

[Nathan Myers]

Oleg Zabluda wrote: 
> Ryszard Kabatek <rysio@rumcajs.chemie.uni-halle.de> wrote:
> : I wrote an iterator class for std::vector<> and std::basic_string<>.
> 
> Let me make some comments. Hopefully it'll spur some useful discussion.
> If the general spirit of my comments will be agreed with, I also
> volunteer to make the nesessary implementation.

Thank you, Oleg.  I strongly agree with the general spirit of your
note.  Some quibbles:

> For example, vector<int>::const_iterator 
> should be a real class, named const_iterator and be a member class of 
> vector<int, .......>, not a typedef to 
> _Iterator<int, ptrdiff_t, const int*, const int&);

I especially agree with this!

> : # ifndef _ITERATOR_FOR_VECTOR_AND_BASIC_STRING_H
> : # define _ITERATOR_FOR_VECTOR_AND_BASIC_STRING_H

There is a convention for include guard naming in the library.

> : template<typename _Tp, typename _Dist, typename _Ptr, typename _Ref>
> : _Iterator<_Tp, _Dist, _Ptr, _Ref>::iterator_category
> : iterator_category(const _Iterator<_Tp, _Dist, _Ptr, _Ref>&);
> 
> : template<typename _Tp, typename _Dist, typename _Ptr, typename _Ref>
> : _Iterator<_Tp, _Dist, _Ptr, _Ref>::value_type*
> : value_type(const _Iterator<_Tp, _Dist, _Ptr, _Ref>&);
> :  
> : template<typename _Tp, typename _Dist, typename _Ptr, typename _Ref>
> : _Iterator<_Tp, _Dist, _Ptr, _Ref>::difference_type*
> : distance_type(const _Iterator<_Tp, _Dist, _Ptr, _Ref>&);
> : *******************************************************************/
> 
> I never saw you use any of these three functions. What are they for?

These are necessary overloads of STL utility functions.  Any 
iterator is supposed to define them, although deriving from 
iterator<> provides them automatically.

> After that, we should also specialize iterator_traits<> for the newly
> defined iterators.
> 
> :     typedef _Iterator<_Tp, _Dist, _Tp*, _Tp&>             iterator;
> :     typedef _Iterator<_Tp, _Dist, const _Tp*, const _Tp&> const_iterator;
> 
> :     typedef _Iterator<_Tp, _Dist, _Ptr, _Ref>             _Self;
> 
> I think _Self should be a private typedef.

I don't see a need for it.  The name "iterator" works just as well in 
the local (iterator class scope) context.

> :     explicit _Iterator(pointer __p);
> 
> I think _Iterator::_Iterator(pointer __p) should be private. Other then
> plain aesthetics, this will also prevent users from writing non-compliant
> code like
> 
> vector<int>::iterator i (0);
 
Agree.   Probably it should take a dummy second argument of a 
type in the restricted namespace.  The only class that should be
able to call the ctor should be vector.
 
> :     template<typename _Up>
> :     _Iterator(const _Iterator<_Tp, _Dist, _Up*, _Up&>& __rhs);
> 
> That's very unsafe. This will provide a conversion not only
> from iterator to const_iterator but many other undesirable
> _implicit_ conversions as well. For example, 
> vector<Derived>::iterator --> vector<Base>::iterator.

No, this is not a conversion from a const_iterator, it's just
a copy of a const reference to an ordinary iterator.

> Making iterator and const_iterator separate classes (see above) 
> will solve this. 

Yes, we do need separate classes for iterator and const_iterator.
Derivation makes this easy: the base class should be named 
const_iterator, and the derived just iterator.  Slicing is your 
friend! :-)

> Also -- general understanding is that Iterators are typically
> designed so that they can be very efficiently passed by value
> (ours are). And they are absolutely not supposed to be used
> polymorphically. I think we should lose passing by reference 
> altogether.

Except of course in the copy constructor.  :-)  
We don't actually need to define one; the language provides
just the right semantics by default.

> This is somewhat controvercial, but I think we should prepend `this->'' 
> in front of all member variables and functions, like this:
> 
>     return *(this->_M_data);
> 
> This will reduce the risk great deal. Imagine somebody doing
> #define _M_data 0      

That #define is strictly forbidden; if users do it, they should expect
breakage.  However, all references to member names not preceded by 
"_M" or "_S" should be qualified this way.  I.e. we try to use
"this->size()" in basic_string<> members in the library.  The 
convention is not followed with any regularity, but should be.
Patches which fix such lapses are welcome.

The reason is not for protection against macros, but to protect against 
surprises in two-phase lookup.  For those of you who have not followed 
comp.std.c++, here is a detailed explanation:

  http://x13.dejanews.com/getdoc.xp?AN=391110976

In the library we have not used "this->" syntax to refer to
"_M_" prefix names, because those will never be global names
and thus cannot be confused with the member names by two-stage
lookup.

When two-phase lookup is implemented lots of code will break.
I think there will be a warning in the next Egcs snapshot for this.
 
Nathan Myers
ncm@cantrip.org