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 11:36:58

Jeffrey Lee Hellrung, Jr. wrote:
> On Mon, Feb 18, 2013 at 5:23 PM, Christian Henning <chhenning_at_[hidden]>wrote:
>
>> On Mon, Feb 18, 2013 at 8:13 PM, Mateusz Loskot <mateusz_at_[hidden]>
>> wrote:
>>> AFAIU, the proposal is to support equivalent to this:
>>>
>>> scanline_read_iterator it =...
>>> std::advance (it, 7); // no I/O performed
>>> auto& buf = *it; // fetch 7th scanline data now
>>>
>>
>> Ok, I just need to increase the position inside operator++. But this
>> brings up an interesting point. What would be the difference_type of a
>> scanline_read_iterator?
>>
>
> _pos is an int (presently), but I'm guessing it can only take nonnegative
> values (other than the sentinel -1 value)? If so, int as the difference
> type should be sufficient, right?
>

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