This is the mail archive of the email@example.com mailing list for the libstdc++ project. See the libstdc++ home page for more information.
Nathan Myers <firstname.lastname@example.org> wrote: : Oleg Zabluda wrote: : > Ryszard Kabatek <email@example.com> 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.