Custom iterator implementation returning OpenCV Mat

Question

I have written an adapter class that allows iteration over the rows of a Mat object from OpenCV. For those interested, here is the Mat documentation, but the salient points are as follows:

• Mat is a reference-counted header to a shared data buffer.
• Mat::row() returns another Mat, which provides a view of a single row of the larger Mat.
• Mat_<T> is a templated version of Mat offering the same interface but better type safety. Mat_<T> and Mat can be converted to one another.

This is my first attempt at writing custom iterators, so I am interested in feedback on correctness and general points to improve. It seems like my use of mutable is suspect, but I don't know if I can avoid it.

Here are the class definitions:

RowRange.hpp

#ifndef CV_ADAPTERS_ROWRANGE_HPP
#define CV_ADAPTERS_ROWRANGE_HPP
#include <iterator>
#include <opencv2/core/core.hpp>
namespace cv
{
template <typename T>
class RowRangeConstIterator : public std::iterator<std::forward_iterator_tag, T>
 {
 public:
 RowRangeConstIterator()
 : data()
 , row()
 , position()
 {}
 RowRangeConstIterator(const cv::Mat_<T>& m, int index)
 : data(m)
 , row()
 , position(index)
 {
 CV_DbgAssert(position >= 0 && position <= data.rows);
 }
 // Dereference
 const cv::Mat_<T>& operator*() const
 {
 setRow();
 return row;
 }
 const cv::Mat_<T>* operator->() const
 {
 setRow();
 return &row;
 }
 // Logical comparison
 bool operator==(const RowRangeConstIterator& that) const
 {
 return this->position == that.position;
 }
 bool operator!=(const RowRangeConstIterator& that) const
 {
 return !(*this == that);
 }
 bool operator<(const RowRangeConstIterator& that) const
 {
 return this->position < that.position;
 }
 bool operator>(const RowRangeConstIterator& that) const
 {
 return this->position > that.position;
 }
 bool operator<=(const RowRangeConstIterator& that) const
 {
 return !(*this > that);
 }
 bool operator>=(const RowRangeConstIterator& that) const
 {
 return !(*this < that);
 }
 // Increment
 RowRangeConstIterator& operator++()
 {
 ++position;
 return *this;
 }
 RowRangeConstIterator operator++(int) const
 {
 RowRangeConstIterator tmp(*this);
 ++(*this);
 return tmp;
 }
 protected:
 void setRow() const
 {
 row = data.row(position);
 }
 cv::Mat_<T> data;
 mutable cv::Mat_<T> row;
 int position;
 };
template <typename T>
class RowRangeIterator : public RowRangeConstIterator<T>
{
public:
 RowRangeIterator()
 : RowRangeConstIterator<T>()
 {}
 RowRangeIterator(const cv::Mat_<T>& m, int index)
 : RowRangeConstIterator<T>(m, index)
 {}
 // Dereference
 cv::Mat_<T>& operator*() const
 {
 this->setRow();
 return this->row;
 }
 cv::Mat_<T>* operator->() const
 {
 this->setRow();
 return &this->row;
 }
};
template <typename T>
class RowRange
{
public:
 typedef RowRangeConstIterator<T> const_iterator;
 typedef RowRangeIterator<T> iterator;
 RowRange(cv::Mat m)
 : data(m)
 {
 CV_Assert(m.type() == cv::DataType<T>::type);
 }
 RowRange(cv::Mat_<T> m)
 : data(m) {}
 const_iterator begin() const
 {
 return const_iterator(data, 0);
 }
 iterator begin()
 {
 return iterator(data, 0);
 }
 const_iterator end() const
 {
 return const_iterator(data, data.rows);
 }
 iterator end()
 {
 return iterator(data, data.rows);
 }
 const_iterator cbegin() const
 {
 return begin();
 }
 const_iterator cend() const
 {
 return end();
 }
private:
 cv::Mat_<T> data;
};
template <typename T>
RowRange<T> make_RowRange(cv::Mat_<T> m)
{
 return RowRange<T>(m);
}
} // namespace cv
#endif /* CV_ADAPTERS_ROWRANGE_HPP */

And an example usage (More examples can be found here):

main.cpp

#include <opencv2/opencv.hpp>
#include "RowRange.hpp"
int main()
{
 cv::Mat m2 = cv::Mat::zeros(3, 3, CV_8UC1);
 for (auto row : cv::RowRange<uchar>(m2))
 {
 row(1) = 255;
 }
}

• Mathieu Guindon: I'm not into C++, but I like how you've named the operator parameters that and go this vs that.
• Roman Shapovalov: Looks handy. Minor thing: data is not the best name for a cv::Mat_ member, since it is the same as the name of the data pointer within cv::Mat; I was confused first.

Answer 1

Disclaimer: I am not a CV expert.

Overall, LGTM.

• Relational operators (==, !=, <, > etc) should not be members but friends. If such operators do not treat their operands symmetrically, expect problems with implicit conversion.

• It is recommended to define operator> in terms of operator<.

• I don't see why row should be a member.

• I understand that position is declared int just because cv::Mat_::rows is int (which is a shame). It'd be nice to have a type of position inferred.

• Last, but not least. This iterator can serve a much wider spectrum of classes than just cv::Mat. Its only dependency on the underlying class is in rows and row methods.

• Aurelius: I made row a member because I'm worried about returning a pointer to a temporary in operator->. I'm pretty sure that returning &data.row(position) will invoke UB. That said, I'm not 100% sure, so I erred on the side of caution. Do you have further thoughts on this? It's the point that I'm most concerned about.
• vnp: As I said, I am not a CV expert. It just looks strange that a member is set on demand. Much more naturally looking is row(data.row(position)) in the constructor.
• Aurelius: Unless I'm misunderstanding you, I can't set row in the constructor, because position can be mutated by the increment operators. Then dereferencing the iterator gives the wrong result.
• sled: @vnp: OpenCV won't copy the data if you do row = data.row(position); it will just return a submatrix of the original matrix, sharing the same piece of memory.