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