Image (Const) Iterator using C++17

Question

I am trying to learn how to correctly implement an iterator (and its corresponding const variant) using a single template, so I would appreciate any criticism to the following code. It's an forward iterator for a cv::Mat wrapper (a class from OpenCV that stores an image).

Edit: Its behavior must be equivalent to a pair of nested fors like for(row in rows) for(colum in columns)

I have already tested it, however I am not familiar with the traditional C++ idioms and patterns so I am not sure that I have properly tested it.

class Image
{
public:
    using value_type = double_t;
    using coord_t    = int32_t;
    static const int value_type_opencv_code = CV_64FC1;

    template <bool isConst>
    class ImageIterator
    {
    public:

        using iterator_category = std::forward_iterator_tag;
        using difference_type   = std::ptrdiff_t;
        using value_type        = Image::value_type;
        using pointer           = std::conditional_t<isConst, value_type const*, value_type*>;
        using reference         = std::conditional_t<isConst, value_type const&, value_type&>;

        ImageIterator(const ImageIterator&) = default;

        template <bool _isConst = isConst, class = std::enable_if_t<_isConst>>
        ImageIterator(const ImageIterator<false>& other) : data(other.data), index(other.index) {}

        ImageIterator(cv::Mat data, size_t initial_index) : data(data), index(initial_index) {}
        
        // There is definitely some code re usage opportunity here
        template <bool _isConst = isConst, class = std::enable_if_t<_isConst>>
        reference operator*() const
        {
            return data.at<value_type>(index_to_row(index), index_to_col(index));
        }

        template <bool _isConst = isConst, class = std::enable_if_t<!_isConst>>
        reference operator*()
        {
            return data.at<value_type>(index_to_row(index), index_to_col(index));
        }

        template<bool _isConst = isConst, class = std::enable_if_t<_isConst>>
        pointer operator->() const
        {
            return data.ptr<value_type>(index_to_row(index)) + index_to_col(index);
        }

        template<bool _isConst = isConst, class = std::enable_if_t<!_isConst>>
        pointer operator->()
        {
            return data.ptr<value_type>(index_to_row(index)) + index_to_col(index);
        }

        ImageIterator<isConst>& operator++()    { ++index; return *this; }
        ImageIterator<isConst>  operator++(int) { ImageIterator<isConst> other(*this); ++(*this); return other; }

        // Not sure if this is acceptable, maybe with friendly free functions
        Image::coord_t getRow()    const { return index_to_row(index); }
        Image::coord_t getColumn() const { return index_to_col(index); }
        auto getCoordinates() const { return std::make_pair(getRow(), getColumn()); }

        friend bool operator==(const ImageIterator<isConst>& a, const ImageIterator<isConst>& b) { return a.index == b.index; }
        friend bool operator!=(const ImageIterator<isConst>& a, const ImageIterator<isConst>& b) { return a.index != b.index; }

        // To allow const_iterator access to iterator's data.
        friend ImageIterator<true>;

    private:
        cv::Mat data; // Maybe add const, if isConst == true
        size_t index;

        // ... [some helper functions] ...
     }
private:
    cv::Mat data;

public:
    Image(cv::Mat& data) : data(data) {};
    Image(coord_t rows, coord_t columns) :
        data(cv::Mat::zeros(cv::Size((int)columns, (int)rows), value_type_opencv_code)) {}
    Image(std::pair<coord_t, coord_t> shape) :
        Image(shape.second, shape.first) {}

    coord_t rows()    const { return data.rows; }
    coord_t columns() const { return data.cols; }
    auto    shape()   const { return std::make_pair(rows(), columns()); }

    value_type  at(const coord_t row, const coord_t column) const { return std::as_const(data).at<double_t>((int)row, (int)column); }
    value_type& at(const coord_t row, const coord_t column)       { return data.at<double_t>((int)row, (int)column); }

    using iterator       = ImageIterator<false>;
    using const_iterator = ImageIterator<true>;

    const_iterator begin() const { return const_iterator(data, 0); }
    const_iterator end()   const { return const_iterator(data, (size_t)rows() * columns()); }
    iterator       begin()       { return iterator(data, 0); }
    iterator       end()         { return iterator(data, (size_t)rows() * columns()); }

    // ... [more irrelevant member functions] ...

    const cv::Mat& toMat() const { return data; }
};

Tests:

// Access using const_iterator
Image::coord_t i{ 0 };
Image::coord_t j{ 0 };
for (const auto& e : std::as_const(image))
{
    EXPECT_EQ(e, generator(i, j));
    // ... [update i and j] ...
}

// Assignment using iterator
double_t i{ 0.0 };
for (auto& e : image)
    e = generator(i++);

// const_iterator and iterator construction from iterator
Image::iterator iter = std::begin(image);
Image::iterator iter2 = iter;
Image::const_iterator citer3 = iter;

while (iter != std::end(image))
{
    EXPECT_EQ(*iter, *iter2);
    EXPECT_EQ(*iter, *citer3);

    ++iter;
    ++iter2;
    ++citer3;
}

I have implemented more tests, however these are the most critical ones. Thanks!

Answer 1

template <bool isConst>
class ImageIterator
{
public:
    ImageIterator(cv::Mat data, size_t initial_index) : data(data), index(initial_index) {}
    ...
private:
    cv::Mat data;
    size_t index;
    ...
};

It looks like cv::Mat is a reference-counted handle type, so copying a cv::Mat doesn't copy the actual data.

However, this also implies that ImageIterator has shared ownership of the data, which would be very unusual in C++. Generally the container (Image) would be the sole owner of the underlying data. An iterator is just a pointer into that data, and keeping one around shouldn't extend the lifetime of the container resources.

So perhaps we should store a pointer to the cv::Mat (or a pointer to the Image class) in the ImageIterator instead.