Re: [boost] [Review] GIL io_new

Subject: Re: [boost] [Review] GIL io_new
From: Roland Bock (rbock_at_[hidden])
Date: 2010-12-11 18:31:58

On 12/11/2010 07:20 PM, Christian Henning wrote:
> Hi Roland, thanks for spending the time.
>
>> I think it would be nice to add iterator pairs and boost::range as
>> additional devices, though.
> I'm thinking of adding input/output iterators for sequential
> read/write. Is that what you are thinking of? How would boost:range
> fit in? Sorry, never used it.
Right, sequential read/write is what I meant.

Concerning Boost.Range: I haven't used it myself, yet, but as far as I 
understand, it is similar to a pair of iterators. I think it is used in 
quite a few boost libraries, that's why I mentioned them.
>>> - What is your evaluation of the implementation?
>> All the code I've looked at so far looks well organized and well readable.
>> Some remarks:
>> * Why are the following files in the detail folder?
>> io_new/detail/read_and_convert_image.hpp
>> io_new/detail/read_and_convert_view.hpp
>> io_new/detail/read_image.hpp
>> io_new/detail/read_image_info.hpp
>> io_new/detail/read_view.hpp
>> io_new/detail/write_view.hpp
> My reason to copy them into detail is to make sure the user isn't
> including those by accident. Make sense to you?
Would any harm come from including them? I thought it is simply not 
necessary since they are included by the format specific include files, 
but if you included them, well, it would still compile but maybe a 
microsecond slower.

> They define functions of the public interface like read_image(). There is no
> need to include them directly, but still, when I want to look at documented
> functions, it seems weird to go into the detail folder...
> Maybe a different folder might help?
That would be better than the detail folder IMHO
> * There seem to be some remnants from earlier versions. For instance:
>   struct tiff_indexed is defined in io_new/tiff_tags.hpp, but never used
> anywhere else (or did I miss something?)
>   io_new/detail/typedefs.hpp ends with } // namespace details<-- notice the
> 's' :-)
> tiff_indexed is a property of tiff that is not supported yet. I should
> remove it for the time being. How did you find out it?
Found it by accident. I wondered where all the tiff things would be used 
and that is the one I started looking for :-)
>>> - What is your evaluation of the documentation?
> [...]
> All noted. Thanks for writing a review!

  :-)