13

I've never implemented STL-like iterators and I try to understand how to implement a very basic thing based on pointers. Once I will have this class I will be able to modify it to do more complicated things. Consequently, this is a first step, and I need it to be rock solid to understand how to write my own iterators (without boost).

I have written the following code and I know that there are errors in it. Can you help me to design correctly a Random Access Iterator class inspired from that :

template<Type> class Container<Type>::Iterator : public std::iterator<random_access_iterator_tag, Type>
{
    // Lifecycle:
    public:
        Iterator() : _ptr(nullptr) {;}
        Iterator(Type* rhs) : _ptr(rhs) {;}
        Iterator(const Iterator &rhs) : _ptr(rhs._ptr) {;}

    // Operators : misc
    public:
        inline Iterator& operator=(Type* rhs) {_ptr = rhs; return *this;}
        inline Iterator& operator=(const Iterator &rhs) {_ptr = rhs._ptr; return *this;}
        inline Iterator& operator+=(const int& rhs) {_ptr += rhs; return *this;}
        inline Iterator& operator-=(const int& rhs) {_ptr -= rhs; return *this;}
        inline Type& operator*() {return *_ptr;}
        inline Type* operator->() {return _ptr;}
        inline Type& operator[](const int& rhs) {return _ptr[rhs];}

    // Operators : arithmetic
    public:
        inline Iterator& operator++() {++_ptr; return *this;}
        inline Iterator& operator--() {--_ptr; return *this;}
        inline Iterator& operator++(int) {Iterator tmp(*this); ++_ptr; return tmp;}
        inline Iterator& operator--(int) {Iterator tmp(*this); --_ptr; return tmp;}
        inline Iterator operator+(const Iterator& rhs) {return Iterator(_ptr+rhs.ptr);}
        inline Iterator operator-(const Iterator& rhs) {return Iterator(_ptr-rhs.ptr);}
        inline Iterator operator+(const int& rhs) {return Iterator(_ptr+rhs);}
        inline Iterator operator-(const int& rhs) {return Iterator(_ptr-rhs);}
        friend inline Iterator operator+(const int& lhs, const Iterator& rhs) {return Iterator(lhs+_ptr);}
        friend inline Iterator operator-(const int& lhs, const Iterator& rhs) {return Iterator(lhs-_ptr);}

    // Operators : comparison
    public:
        inline bool operator==(const Iterator& rhs) {return _ptr == rhs._ptr;}
        inline bool operator!=(const Iterator& rhs) {return _ptr != rhs._ptr;}
        inline bool operator>(const Iterator& rhs) {return _ptr > rhs._ptr;}
        inline bool operator<(const Iterator& rhs) {return _ptr < rhs._ptr;}
        inline bool operator>=(const Iterator& rhs) {return _ptr >= rhs._ptr;}
        inline bool operator<=(const Iterator& rhs) {return _ptr <= rhs._ptr;}

    // Data members
    protected:
        Type* _ptr;
};

Thank you very much.

Vincent
  • 57,703
  • 61
  • 205
  • 388
  • 2
    What errors? Compiler errors? Linker errors? Runtime errors? Logical errors? Something else? – Some programmer dude Aug 23 '12 at 13:19
  • "I know there are errors" -- is this some sort of test? I don't think SO is the right site for that... – Kerrek SB Aug 23 '12 at 13:19
  • A pointer is an iterator. What is the purpose of wrapping it with a class? – Pete Becker Aug 23 '12 at 13:19
  • The purpose is to understand how to correctly implement an iterator class (particularly the function signatures and what the function do). – Vincent Aug 23 '12 at 13:27
  • 1
    Well you don't really need `public` all over the place, but as you have no data members in them it makes no difference. `return Iterator(_ptr+rhs);` in theory creates an extra unnecessary temporary, you could just `return _ptr+rhs;`. – BoBTFish Aug 23 '12 at 13:33
  • Except for the assignment operators (including `+=`, `-=`), all of the operators should be const-qualified member functions. – James McNellis Aug 23 '12 at 21:29

3 Answers3

15

Your code has the following issues:

  • You do not follow the Rule of Three/Five. The best option in your situation is not to declare any custom destructors, copy/move constructors or copy/move assignment operators. Let's follow so called Rule of Zero.
  • Iterator(Type* rhs) could be private and the Container could be marked as Iterator's friend, but that's not strictly necessary.
  • operator=(Type* rhs) is a bad idea. That's not what type safety is about.
  • Post-in(de)crementation should return Iterator, not Iterator &.
  • Adding two iterators has no meaning.
  • Subtracting two iterators should return a difference, not a new iterator.
  • You should use std::iterator<std::random_access_iterator_tag, Type>::difference_type instead of const int &.
  • If a method does not modify an object, it should be marked const.

Useful resource: RandomAccessIterator @ cppreference.com

Here is a fixed version of your code:

template<typename Type>
class Container<Type>::Iterator : public std::iterator<std::random_access_iterator_tag, Type>
{
public:
    using difference_type = typename std::iterator<std::random_access_iterator_tag, Type>::difference_type;
    
    Iterator() : _ptr(nullptr) {}
    Iterator(Type* rhs) : _ptr(rhs) {}
    Iterator(const Iterator &rhs) : _ptr(rhs._ptr) {}
    /* inline Iterator& operator=(Type* rhs) {_ptr = rhs; return *this;} */
    /* inline Iterator& operator=(const Iterator &rhs) {_ptr = rhs._ptr; return *this;} */
    inline Iterator& operator+=(difference_type rhs) {_ptr += rhs; return *this;}
    inline Iterator& operator-=(difference_type rhs) {_ptr -= rhs; return *this;}
    inline Type& operator*() const {return *_ptr;}
    inline Type* operator->() const {return _ptr;}
    inline Type& operator[](difference_type rhs) const {return _ptr[rhs];}
    
    inline Iterator& operator++() {++_ptr; return *this;}
    inline Iterator& operator--() {--_ptr; return *this;}
    inline Iterator operator++(int) const {Iterator tmp(*this); ++_ptr; return tmp;}
    inline Iterator operator--(int) const {Iterator tmp(*this); --_ptr; return tmp;}
    /* inline Iterator operator+(const Iterator& rhs) {return Iterator(_ptr+rhs.ptr);} */
    inline difference_type operator-(const Iterator& rhs) const {return _ptr-rhs.ptr;}
    inline Iterator operator+(difference_type rhs) const {return Iterator(_ptr+rhs);}
    inline Iterator operator-(difference_type rhs) const {return Iterator(_ptr-rhs);}
    friend inline Iterator operator+(difference_type lhs, const Iterator& rhs) {return Iterator(lhs+rhs._ptr);}
    friend inline Iterator operator-(difference_type lhs, const Iterator& rhs) {return Iterator(lhs-rhs._ptr);}
    
    inline bool operator==(const Iterator& rhs) const {return _ptr == rhs._ptr;}
    inline bool operator!=(const Iterator& rhs) const {return _ptr != rhs._ptr;}
    inline bool operator>(const Iterator& rhs) const {return _ptr > rhs._ptr;}
    inline bool operator<(const Iterator& rhs) const {return _ptr < rhs._ptr;}
    inline bool operator>=(const Iterator& rhs) const {return _ptr >= rhs._ptr;}
    inline bool operator<=(const Iterator& rhs) const {return _ptr <= rhs._ptr;}
private:
    Type* _ptr;
};
  
The Shmoo
  • 358
  • 1
  • 16
cubuspl42
  • 7,833
  • 4
  • 41
  • 65
2

In general your approach is right. The postfix increment/decrement operator should return by value, not by reference. I also have doubts about:

Iterator(Type* rhs) : _ptr(rhs) {;}

This tells everyone that this iterator class is implemented around pointers. I would try making this method only callable by the container. Same for assignment to a pointer. Adding two iterators makes no sense to me (I would leave "iterator+int"). Substracting two iterators pointing to the same container might make some sense.

Community
  • 1
  • 1
Alexander Chertov
  • 2,070
  • 13
  • 16
  • As he said, this is an educational exercise, that's why he's going the whole hog. I was surprised about `operator[]` too, but it's there for random access iterators! http://en.cppreference.com/w/cpp/concept/RandomAccessIterator – BoBTFish Aug 23 '12 at 13:37
  • I believe you need `inline difference_type operator-(const Iterator&)` instead of `inline Iterator operator-(const Iterator&)`. I don't believe you need the same for `operator+`. Some algorithms (like `std::sort`) don't seem to work without the `difference_type operator-`. You can't have both because only the return types differ. – greg Feb 14 '14 at 12:44
2

Have a look at how Boost do it, the iterators in boost/container/vector.hpp - vector_const_iterator and vector_iterator are reasonably easy to understand pointer based iterators.

Silas Parker
  • 8,017
  • 1
  • 28
  • 43