Re: [boost] [Review] GIL io_new

Subject: Re: [boost] [Review] GIL io_new
From: Christian Henning (chhenning_at_[hidden])
Date: 2010-12-11 13:20:01

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.

>
>> - 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?

>
> 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?

>
> * 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?

>
>>- What is your evaluation of the documentation?

>
> The documentation is well readable and easy to understand.
>
> There seem to be broken links:
>  * "section Extending GIL::IO with new Formats" links to index.html
>  * "Please see section Supported Image Formats." links to index.html
>  * "sections _Supported_Formats_ for more details." links to /
>
> It would be good to have code samples for read_and_convert_image, read_view
> and read_and_convert_view in the tutorial. The "Using IO" section seems to
> assume that they are already documented: "As the Tutorial demonstrated there
> are a few ways to read images."
>
> Additionally, it would be great to have a reference section. :-)
>
> I stumbled over a few typos and grammatically unusual sentences (IMHO):
>  * s/supoorted/supported/
>  * "Before we can read or write image one thing." I know what that means in
> German, but does this make sense in English?
>  * "If the user doesn't know what he is dealing with it can use
> read_and_convert_image()." s/it/he/

All you items are noted and will be worked on. Thanks for parsing.

>
> I think it would make much sense to give the text to an English native
> speaker.

Yes, it would.

>> - Do you think the extensions should be accepted as a part of Boost.GIL
> library?
>
> Yes, with a few wishes (see above):
>  - documentation:
>    - typos and grammar
>    - add reference section
>    - add a few more code samples in the tutorial
>  - file structure
>    - move parts of the public interface into the non-detail area
>  - interface:
>    - add support for iterators and ranges
>
>

All noted. Thanks for writing a review!

Regards,
Christian