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?
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
Yes, it would.
>> - Do you think the extensions should be accepted as a part of Boost.GIL
> 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!
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk