Iterator class for vector and basic_string

[Nathan Myers]

Oleg wrote: 
> Nathan Myers <ncm@cygnus.com> wrote:
> : Oleg Zabluda wrote: 
> : > Ryszard Kabatek <rysio@rumcajs.chemie.uni-halle.de> wrote:
> : > : I wrote an iterator class for std::vector<> and std::basic_string<>.
>  ...
> : > :     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.  
> 
> Why? It will be ``explicit'' and private. What am I missing?

Private constructors still participate in overloading.

> : The only class that should be
> : able to call the ctor should be vector.
> 
> And possibly the const cousin.

Right.

> : > :     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.
> 
> I think you are mistaken. 

Of course you are right.

> BTW. Now that I look at this again, I see other problems with
> this piece of code. First of all, technicaly, this is not just 
> a copy constructor, therefore the comment is incorrect. Second of all, 
> there is no matching [non-copy] assignment operator. 

In fact it's not a copy constructor at all; given this definition the 
compiler would still automatically generate a copy constructor.  I agree 
that we don't want any promiscuously polymorphic copy constructors for 
vector and string, because the types they store cannot be used 
polymorphically.  

The assignment would also be compiler-generated.

> Third of all, -Weffc++ will complain that we have a pointer
> data member, but do not define our own copy constructor and
> copy assignment. I am afraid, currently we have no choice but to 
> define them, if we want to have any hope to ever have our headers be 
> -Weffc++ clean. Amen. I am afraid though that this might lead 
> to a performance penalty of some sort. Comments anyone?

I don't give a rip about -Weffc++ cleanliness.  Those rules are not
generally-applicable enough to apply it uniformly to the standard
library.  I don't mind running things past it to see what happens,
but I refuse to be bound by it.  Scott Meyers conflates "OO design"
with "C++ coding", but they are different processes with different
rules.  When you're not coding an OO design, OO rules don't 
necessarily apply.

> The ideal solution would be to surgically surpress those warnings
> with some king of #pragma, but the last time Mark Mitchell was 
> proposing introducing them, other disagreed that it would be worth it.

I agree.  They are just guidelines.

> : > 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! :-)
> 
> Actually, it is always your enemy :-) I can't think of a single
> example where it would be your friend.
> 
> We should not derive iterator from const_iterator.
> In this particular case, it is impossible to do that and stay
> -Weffc++ clean, because, for example, you will not be able
> to override operator*(). At best you will hide it. What kind
> of derivation is that? Moreover, making the two really, really
> different classes, we can make the pointer data member in the
> const_iterator to be a real pointer to const. Besides, if
> we derive one from the other, and not explicitly ban the slicing, 
> we will introduce other bizarre _implicit_ conversions, like
> iterator* --> const_iterator* (compare to T** --> const T**).
> 
> In short, we'll never drive Plauger out of business if we
> gonna repeat his mistakes :-)

OK, agreed.  I hadn't studied his mistakes beyond his having
derived in the wrong direction (!).  I would like to do some
experiments with private derivation and some "using" exposures.  
The nice properties of built-in promotion are not to be given
up lightly.

> We could derived them from a common base, but this will
> introduce a vtbl and we don't want that.

Right, maybe.  Anyway, for something as simple as a pointer
wrapper I'm not sure implementation-inheritance would buy 
you anything anyway, besides built-in promotion.

> : > 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.
> 
> Right. We shouldn't. Unfortunately we might be forced to. See above.

A converting constructor, OK.  But that's different.

> : > 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.  
> 
> Right. But if we can help it, with absolutely no downside, why
> not? Especially since, strictly speaking, we invade namespace
> of other compilers. 

If they define _M_data, all kinds of things break.  That's the
last thing we can afford to worry about.  But still, it would
be no bad thing to use this-> for member access.

> : 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.  
> 
> Right. I just thought that macro was a simpler example. The
> case in question seemed specially heinous, because that would
> lead to returning a null pointer from a valid iterator, core
> dumps, and absolutely no sane way to debug the problem that 
> I can think of.

That would be the least of the problems if somebody did that.
Anyway, any place where we had "_M_data = __p" would break too,
and visibly.

> : 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.
> 
> Unless the user will. Or some other libraries will. Like I said,
> it doesn't cost us anything, so why not?

Users and other libraries are not allowed to define _M_ names.
Of course vendor C headers might, but if we did then "this->" 
wouldn't help us anyway.  Take a look in BADNAMES.  More will
be added to this, I am sure.

> : 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.
> 
> I am pretty scared of the name lookup issues myself. ``this->''
> makes me feel somewhat better. But I don't assume this is
> universal, and would not insist, unless enough of other people 
> feel the same way.

Personally, I think we should accept patches which add "this->"
to template member name usage, but I will find it hard to add
it universally.  That may change.

Nathan Myers
ncm@cantrip.org