Writing stl compatible iterators

Question

I'm trying to convert an iterator class I have to be stl compatible so that it can be used with the stl algorithms. In the following simple (and frankly useless) example, which should print the values 0 to 5 inclusive, I am getting the following errors,

ISO C++ forbids incrementing a pointer of type ‘Iterator (*)()‘

and,

invalid conversion from ‘Iterator (*)()’ to ‘int‘

What am I doing wrong?

Thanks.

---

#include <iterator>
#include <algorithm>
#include <iostream>

class Iterator : public std::iterator<std::bidirectional_iterator_tag, int> {
    public:
        Iterator(int i = 0) : val(i) {
            if(val<0 || val>5) throw;
        }

        bool operator==(Iterator const& rhs) const {
            return (val==rhs.val);
        }

        bool operator!=(Iterator const& rhs) const {
            return !(*this==rhs);
        }

        Iterator& operator++() {
            if(val!=6)
                ++val;
            return *this;
        }   

        Iterator operator++(int) {
            Iterator tmp (*this);
            ++(*this);
            return tmp;
        }

        Iterator& operator--() {
            if(val!=-1)
                --val;
            return *this;
        }

        Iterator operator--(int) {
            Iterator tmp (*this);
            --(*this);
            return tmp;
        }

        int operator* () const {
            if(val==-1 || val==6) throw;
            return val;
        }

    private:
        int val;
};

Iterator begin() {
    return Iterator();
}

Iterator end() {
    return ++Iterator(5);
}

void print(int i) {
    std::cout << i << std::endl;
}

int main(int argc, char* argv[]) {
    std::for_each(begin,end,print);
}

Answer 1

You are passing the functions begin and end to std::for_each, instead of the iterators that these functions would return:

std::for_each(begin,end,print);

It should be:

std::for_each(begin(),end(),print);

Also note that the empty throw statements, like in if(val==-1 || val==6) throw;, will not do anything good. You have to throw something, like throw std::out_of_range("out of bounds").

Answer 2

First, you should be passing the iterators that are returned by begin() and end() not the functions themselves:

int main(int argc, char* argv[]) 
{
    std::for_each(begin(),end(),print);
}

Secondly, it would be useful to have a templated Iterator class:

template<class T>
class Iterator : public std::iterator<std::bidirectional_iterator_tag, int>
{
    public:
        typedef T value_type; //notice this here :D

        Iterator(value_type t = 0) : val(t) 
        {
            if(val<0 || val>5) throw; //never hardcode something like that :S
        }

        bool operator==(Iterator const& rhs) const 
        {
            return (val==rhs.val);
        }

        bool operator!=(Iterator const& rhs) const 
        {
            return !(*this==rhs);
        }

        Iterator& operator++() 
        {
            if(val!=6) //never hardcode something like that :S
                ++val;
            return *this;
        }   

        Iterator operator++(value_type) 
        {
            Iterator tmp (*this);
            ++(*this);
            return tmp;
        }

        Iterator& operator--() 
        {
            if(val!=-1) //never hardcode something like that :S
                --val;
            return *this;
        }

        Iterator operator--(value_type) 
        {
            Iterator tmp (*this);
            --(*this);
            return tmp;
        }

        value_type operator* () const 
        {
            if(val==-1 || val==6) throw; //never hardcode something like that :S
            return val;
        }

    private:
        value_type val;
};

Third, you might not really want to have an iterator class on its own like that. Here is an example of what you can do (note an iterator class is a bit lower) :

#include <algorithm>

template<class T>
class List
{
public:
    typedef T value_type;
    typedef std::size_t size_type;

private:
    struct Knot
    {
        value_type val_;
        Knot * next_;
        Knot(const value_type &val)
        :val_(val), next_(0)
        {}
    };
    Knot * head_;
    size_type nelems_;

public:
    //Default constructor
    List() throw()
    :head_(0), nelems_(0)
    {}
    bool empty() const throw()
    { return size() == 0; }
    size_type size() const throw()
    { return nelems_; }

private:
    Knot * last() throw() //could be done better
    {
        if(empty()) return 0;
        Knot *p = head_;
        while (p->next_)
            p = p->next_;
        return p;
    }

public:
    void push_back(const value_type & val)
    {
        Knot *p = last();
        if(!p)
            head_ = new Knot(val);
        else
            p->next_ = new Knot(val);
        ++nelems_;
    }
    void clear() throw()
    {
        while(head_)
        {
            Knot *p = head_->next_;
            delete head_;
            head_ = p;
        }
        nelems_ = 0;
    }
    //Destructor:
    ~List() throw()
    { clear(); }
    //Iterators:
    class iterator
    {
        Knot * cur_;
    public:
        iterator(Knot *p) throw()
        :cur_(p)
        {}
        bool operator==(const iterator & iter)const throw()
        { return cur_ == iter.cur_; }
        bool operator!=(const iterator & iter)const throw()
        { return !(*this == iter); }
        iterator & operator++()
        {
            cur_ = cur_->next_;
            return *this;
        }
        iterator operator++(int)
        {
            iterator temp(*this);
            operator++();
            return temp;
        }
        value_type & operator*()throw()
        { return cur_->val_; }
        value_type operator*() const
        { return cur_->val_; }
        value_type operator->()
        { return cur_->val_; }
        const value_type operator->() const
        { return cur_->val_; }
    };
    iterator begin() throw()
    { return iterator(head_); }
    iterator begin() const throw()
    { return iterator(head_); }
    iterator end() throw()
    { return iterator(0); }
    iterator end() const throw()
    { return iterator(0); }
    //Copy constructor:
    List(const List & lst)
    :head_(0), nelems_(0)
    {
        for(iterator i = lst.begin(); i != lst.end(); ++i)
            push_back(*i);
    }
    void swap(List & lst) throw()
    {
        std::swap(head_, lst.head_);
        std::swap(nelems_, lst.nelems_);
    }
    List & operator=(const List & lst)
    {
        List(lst).swap(*this);
        return *this;
    }
    //Conversion constructor
    template<class U>
    List(const List<U> &lst)
    :head_(0), nelems_(0)
    {
        for(typename List<U>::iterator iter = lst.begin(); iter != lst.end(); ++iter)
            push_back(*iter);
    }
    template<class U>
    List & operator=(const List<U> &lst)
    {
        List(lst).swap(*this);
        return *this;
    }
    //Sequence constructor:
    template<class Iter>
    List(Iter first, Iter last)
    :head_(0), nelems_(0)
    {
        for(;first!=last; ++first)
            push_back(*first);
    }
};

#include <iostream>
using std::cout;
using std::endl;

int main()
{
    const char MAX_LIMIT = 127;
    List<short> listA;
    //...
    List<char> listB = listA; //without the conversion constructor this would not go very far!
    for(char i = 0; i < MAX_LIMIT; ++i)
        listB.push_back(i);
    for(List<char>::iterator iter = listB.begin(); iter != lstB.end(); ++iter)
        cout << *iter << endl;
}