Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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;
    }
}
share|improve this question
1  
I'm not into C++, but I like how you've named the operator parameters that and go this vs that. –  Mat's Mug Jul 18 '14 at 2:45

1 Answer 1

up vote 2 down vote accepted
+50

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.

share|improve this answer
    
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. –  Aurelius Jul 29 '14 at 5:57
    
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. –  vnp Jul 29 '14 at 19:48
    
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. –  Aurelius Jul 29 '14 at 20:23

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.