Cannot get BOOST_FOREACH to work with my custom class

Question

I implemented a trivial class MyClass that has an array allocated with new inside it (I know that I could use a STL container, but I'm trying to understand how they work). I also created an iterator subclass, able to iterate on all the elements of a MyClass object:

class MyIterator : public iterator<forward_iterator_tag,int>
{
private:
    int* data= nullptr;
    int length;
    int pointer=0;
    int nullvalue=0;
public:
    MyIterator(int* data, int length, bool end= false)
    {
        this->data= data;
        this->length= length;
        if(end)
            pointer=-1;
    }
    ~MyIterator()
    {
        delete[] data;
    }
    MyIterator& operator++()
    {
        if(pointer!= length-1)
        {
            pointer++;
        }
        else
        {
            pointer= -1;
        }
        return *this;
    }
    bool operator==(const MyIterator& other)
    {
        return pointer==other.pointer;
    }
    bool operator!=(const MyIterator& other)
    {
        return pointer!= other.pointer;
    }
    int& operator* ()
    {
        if(pointer==-1)
            return nullvalue;
        else
            return data[pointer];
    }
};

class MyClass
{
private:
    int* data= nullptr;
    int length= 100;
public:
    MyClass()
    {
        data= new int[length];
        for(int i=0; i<length;i++)
            data[i]=i+1;
    }
    iterator<forward_iterator_tag,int> begin()
    {
        return MyIterator(data,length);
    }
    iterator<forward_iterator_tag,int> end()
    {
        return MyIterator(data,length,true);
    }
};

While the iterator works if I use it this way:

for(MyIterator i= MyClass_instance.begin(); i!=MyClass_instance.end();i++) {...}

It doesn't work if I try to use it with BOOST_FOREACH:

BOOST_FOREACH(int i, MyClass_instance) {...}

These are the errors that I get:

Answer 1

• You're slicing your iterator by returning them as std::iterator<> by value. You cannot do that.

  Returning by reference would avoid the slicing problem but introduces a worse problem: it returns a reference to a temporary¹.

  Hence, fix it by returning your actual iterator type by value.

• Your type was missing a const iterator.

• All the iterator members weren't const-correct.

• Also, according to the page Extensibility it looks like you need to add

  namespace boost {
      template<> struct range_mutable_iterator<MyClass> {
          typedef MyClass::MyIterator type;
      };
  
      template<> struct range_const_iterator<MyClass> {
          typedef MyClass::MyConstIterator type;
      };
  }
  

• There is serious trouble with the Rule-Of-Three implementation for your iterator type (What is The Rule of Three?).

  You're deleting the container's data every time an iterator goes out of existence. And MyClass itself never frees the data...

Fixing most (?) of the above:

Live On Coliru

#include <iterator>
#include <boost/foreach.hpp>

class MyClass
{
private:
    int* data  = nullptr;
    int length = 100;

public:
    class MyIterator : public std::iterator<std::forward_iterator_tag, int>
    {
        public:
            MyIterator(int* data, int length, bool end = false)
            {
                this->data= data;
                this->length= length;
                if(end)
                    pointer=-1;
            }
            MyIterator& operator++()
            {
                if(pointer!= length-1) {
                    pointer++;
                }
                else {
                    pointer= -1;
                }
                return *this;
            }

            bool operator==(const MyIterator& other) const { return pointer==other.pointer; }
            bool operator!=(const MyIterator& other) const { return pointer!= other.pointer; }
            int& operator*() const
            {
                if(pointer==-1)
                    return nullvalue;
                else
                    return data[pointer];
            }
        private:
            value_type* data     = nullptr;
            int length;
            int pointer          = 0;
            mutable value_type nullvalue = 0;
    };

    class MyConstIterator : public std::iterator<std::forward_iterator_tag, const int>
    {
        public:
            MyConstIterator(int const* data, int length, bool end = false)
            {
                this->data= data;
                this->length= length;
                if(end)
                    pointer=-1;
            }
            MyConstIterator& operator++()
            {
                if(pointer!= length-1) {
                    pointer++;
                }
                else {
                    pointer= -1;
                }
                return *this;
            }

            bool operator==(const MyConstIterator& other) const { return pointer==other.pointer; }
            bool operator!=(const MyConstIterator& other) const { return pointer!= other.pointer; }
            int const& operator*() const
            {
                if(pointer==-1)
                    return nullvalue;
                else
                    return data[pointer];
            }
        private:
            value_type* data     = nullptr;
            int length;
            int pointer          = 0;
            value_type nullvalue = 0;
    };

public:
    typedef MyIterator iterator_type;
    typedef MyConstIterator const_iterator_type;

    MyClass()
    {
        data= new int[length];
        for(int i=0; i<length;i++)
            data[i]=i+1;
    }
    ~MyClass() {
        delete[] data;
    }
    iterator_type begin()             { return MyIterator(data,length);      }
    iterator_type end()               { return MyIterator(data,length,true); }
    const_iterator_type begin() const { return MyConstIterator(data,length);      }
    const_iterator_type end()   const { return MyConstIterator(data,length,true); }
};

namespace boost {
    template<> struct range_mutable_iterator<MyClass> {
        typedef MyClass::MyIterator type;
    };

    template<> struct range_const_iterator<MyClass> {
        typedef MyClass::MyConstIterator type;
    };
}

#include <iostream>

int main()
{
    MyClass c;
    BOOST_FOREACH(int i, c) {
        std::cout << i << "\n";
    }
}

---

¹ (unless you store the iterators somewhere else, but that would be a huge anti-pattern for many many reasons)