3
\$\begingroup\$

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!

\$\endgroup\$
1
  • 1
    \$\begingroup\$ Try using Boost.Iterator, specifically the iterator_facade template. \$\endgroup\$ – JDługosz May 10 at 14:03
1
\$\begingroup\$
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.

\$\endgroup\$
1
  • \$\begingroup\$ 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. \$\endgroup\$ – Black Arrow May 10 at 23:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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