Re: [boost] [gil::io] Feedback for scanline_read_iterator

Subject: Re: [boost] [gil::io] Feedback for scanline_read_iterator
From: Michael Marcin (mike.marcin_at_[hidden])
Date: 2013-02-20 21:36:52

Michael Marcin wrote:
>
> I'm not sure exactly what _reader->clean_up(); does but the destructor
> seems likely wrong. If you make a copy of the iterator and the original
> or the copy gets destructed it calls clean_up while the other copy may
> still be in use. Shouldn't clean_up just be part of the scanline_reader
> destructor and not the iterator?
>
> The set_reader and set_buffer member functions seem unnecessary and
> confuse things.
>
> I think the checks at the top of the dereference operator are
> unnecessary. You're reporting programming errors with exceptions.
>
> if( _reader == NULL ) { io_error( "Reader cannot be null for this
> operation." ); }
> if( _buffer == NULL ) { io_error( "Buffer cannot be null for this
> operation." ); }
>
> Same in increase_pos
> if( this->_reader == NULL ) { io_error("Reader cannot be null for this
> operation."); }
>
> The process of generating the buffer and supplying it to the iterator
> feels strange to me but I could be wrong.
>
> There is an equal member function that has nothing to do with
> operator==. Seems like equal should just go away.
>
> I don't see why end has to be constructed in that manner. It would seem
> to me that an end iterator constructed with the reader makes the
> implementation a whole lot simpler. I would prefer an interface where
> you just call reader.begin() and reader.end() to get your iterators.
> You'll no longer need a sentinel value for past-the-end, _pos could be
> initialized to reader->_info._height.
>
> Speaking of which is _info._height part of the Reader concept? Is the
> Reader concept documented anywhere I couldn't find it but I only did a
> cursory search.
>
> I don't really understand the second parameter to _reader->skip and
> _reader->read. It seems to be ignored for everything except the TIFF
> reader. Which leads me to believe that if TIFF reader needs to keep
> track of its current row it should do so internally as all the other
> readers track their internally (seemingly implicitly). Also I'm not sure
> but given the interface it seems that TIFF skip could maybe be a noop if
> the row is passed to read still or just a simple increment of an
> internal row counter.
>
>
> Note also that I think the requirement on InputIterator concept that
> Postincrement and dereference (*i++) be valid forces lazy evaluation. If
> you did eager reading/skipping somehow in increment you'd break that
> expression.
>
> Here is a sketch of a possible implementation.
> http://codepad.org/9hjn3SpQ
>

I don't particularly love the class name.

An istream_iterator iterates over the elements of the stream.
A directory_iterator iterates over the contents of a directory.
A vector::iterator iterates over the elements of the vector.

A scanline_read_iterator iterates over the scanlines of an image.

One might reasonably expect a scanline_read_iterator to iterate over the 
elements of a scanline.

I could be wrong and I don't really have a better name aside from maybe 
scanline_reader::iterator. (reader_t::iterator in your example code)

Also scanline_read_iterator.hpp is in the detail folder. If this class 
isn't meant for public consumption completely ignore my naming concerns.