Subject: Re: [boost] GIL IO extension review
From: Christian Henning (chhenning_at_[hidden])
Date: 2010-12-06 13:56:03
Hi Kenny, thanks a lot for your review.
On Mon, Dec 6, 2010 at 12:52 PM, Kenny Riddile <kfriddile_at_[hidden]> wrote:
> Sorry if this review seems short, but most of my time is spoken for. Still,
> I think this extension is useful enough that I found time to review it.
> I've been using the new version of the GIL IO extension for over a year now
> and am glad to finally see it up for review. I've primarily used it for
> loading various image formats for use as OpenGL textures. I recently gained
> some experience with extending the library to support an additional image
> format (TARGA) as well.
I'll add the targa code as soon as I can.
> - What is your evaluation of the implementation?
> I hadn't looked at the implementation much prior to recently adding support
> for reading and writing TARGA images, and my evaluation of the
> implementation is from that standpoint. The fact that I (of all people) was
> able to easily add support for a new format without much documentation on
> the subject suggests that the relevant parts of the implementation are
> easily understandable. I mostly used the BMP implementation as a reference,
> and didn't notice anything egregious or suspicious. I have some concerns
> about the usage of setjmp/longjmp in the PNG and JPEG implementations
> (honestly I get suspicious any time I see those). I think there was
> discussion elsewhere about adding additional test cases to exercise the
> error cases that result in a longjmp so that all of the supported platforms
> can be verified. I think this is a good idea. Another nitpick that came up
> when adding TARGA support are the read_int8, read_int16, read_int32,
> write_int8, etc. member functions of the Device models in io_device.hpp. To
> me, these names suggest that these functions return and accept signed
> integers when in reality they return and accept unsigned integers.
The setjmp/longjmp solution is the best have come up with. If there is
a better way I like to know.
Regarding the read_intxx functions I could change that.
> - What is your evaluation of the documentation?
> The documentation was sufficient enough for me to quickly figure out how to
> read and write images from all of the supported formats. The section on
> extending the library to support new formats was enough to get me started,
> but I ended up just relying on the existing BMP code as a reference. This
> suggests that maybe that section of the documentation should be fleshed out
> a little more.
Yes, the documentation has to be updated a bit. I have to add a more
boost specific layout for instance. That will be done before I add it
the trunk, in case the review is successful
Thanks again for your time,
Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk