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