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!

• JDługosz: Try using Boost.Iterator, specifically the iterator_facade template.

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.

• Black Arrow: You're right, it makes no sense for an iterator to own a resource. I did not notice that. I guess I'll go with a pointer (or a reference) to cv::Mat. High cohesiveness isn't a problem between a container and its iterator.