[boost] [Review] GIL io_new

Subject: [boost] [Review] GIL io_new
From: Roland Bock (rbock_at_[hidden])
Date: 2010-12-11 10:35:52

Hi,

here is a short review of GIL new IO (I missed the documentation of the 
toolbox, which is why I concentrated on IO).

 > - What is your evaluation of the design?
Quite elegant and intuitive.

I think it would be nice to add iterator pairs and boost::range as 
additional devices, though.

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

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

* 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' :-)

 >- What is your evaluation of the documentation?
As stated above, I missed the documentation on the toolbox.

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/

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

 > - What is your evaluation of the potential usefulness of the extensions?
Very useful!

 > - Did you try to use the extensions? With what compiler? Did you have
any problems?
I did not actually build anything. Sorry :-(
But I am looking forward to doing so!

 > - How much effort did you put into your evaluation? A glance? A quick
 > - reading? In-depth study?
About half a day of reading.

 > - Are you knowledgeable about the problem domain?
I have used several other libraries over the last 15 years, but I never 
delved into the innards of image file formats...

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

Again, I did not evaluate the toolbox part.

Thanks writing this extension!

Roland