Boost logo

Boost :

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.

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.

Boost list run by bdawes at, gregod at, cpdaniel at, john at