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