11 Dec
2010
11 Dec
'10
4:35 p.m.
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
11 Dec
11 Dec
7:20 p.m.
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
12 Dec
12 Dec
12:31 a.m.
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! :-)
5439
Age (days ago)
5439
Last active (days ago)
2 comments
2 participants
participants (2)
-
Christian Henning -
Roland Bock