This is the mail archive of the libstdc++@sourceware.cygnus.com mailing list for the libstdc++ project. See the libstdc++ home page for more information.


[Date Prev][Date Next][Thread Prev][Thread Next]
[Date Index] [Subject Index] [Author Index] [Thread Index]

Re: Iterator class for vector and basic_string



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?

: The only class that should be
: able to call the ctor should be vector.

And possibly the const cousin.

: > :     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. It's a template constructor whose
purpose seems to be an implicit conversion from iterator to
const_iterator. The comments in the original code say this
(and I quote)

   // template copy constructor for conversion
   // iterator <-- const_iterator.
   template<typename _Up>
      _Iterator(const _Iterator<_Tp, _Dist, _Up*, _Up&>& __rhs);

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. 

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?

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.

: > 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 :-)

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

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

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

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

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

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

Oleg.