Optimizations / fixes

[kleisauke]

Hi John,

I had some time to cook up a PR (as requested in https://github.com/jcupitt/lua-vips/issues/24). For easier reviewing, I've split up the changes in multiple commits.

You could also consider to remove the vips_argument_map callback and change it to a pull-style API in libvips (because callbacks are slow in LuaJIT, see here). I already tried to do this (see https://github.com/kleisauke/lua-vips/commit/6d451f0493b788007fafeaf5a53c74547a0430d6), but haven't included it in this PR, because I'm not sure how the API should look like (and I'm not happy with that VipsArgumentNameFlagsArray struct, I think you'll find a better way).

This PR includes

Code improvements

• Separate C declarations into one file (easier to maintain and corresponds to pyvips).
• Use dot syntax as path separator for requiring modules (seems more common in Lua).
• Initialize libvips with the name of the application.
• Use vips_value_set_blob_free for libvips 8.6+ (because callbacks are slow in LuaJIT).
• Handle refstr separately in get() (see: https://github.com/jcupitt/pyvips/commit/ffe8c70f469bd7217f35648a64ef5898c3516786).
• Improve the metatable logic (removes suspicious setmetatable(Image, Image)).
• Use the colon operator where possible.
• Use return values where possible.
• More convenient (and more OOP-style) to declare instance methods.
• Remove unused declarations.
• Delete unneeded iteration in voperation.
• Don't throw errors when the type does not exist (the caller is responsible for this).
• Remove unnecessary self log.
• Speed-up voperation:
  • Cache the length of (call-)arguments.
  • Avoid unnecessary loops.
  • Remove table.insert (replace it by using the # operator instead).
• Optimize with the help of local variables (see: http://lua-users.org/wiki/OptimisingUsingLocalVariables).

Bug fixes

• Fix undefined reference to vips_error_clear().
• Check that logging is enabled for msg_r and print_r (avoid calling prettyprint_table).

Unit tests improvements

• Improve error checking (the error may vary for different giflib versions).

[jcupitt]

Oh wow, that was quick. OK, I'll try to read through this.

[jcupitt]

That's great! It looks a lot better for you having polished it a bit.

I made some small comments -- the only big thing is "should get() / set() throw an exception?"

Exception handling is a little awkward in Lua, so I can see an argument for a simple error return. The idea was to have get_typeof() and set_type() as the error-return ones, andget()/set()` as the exception ones, but that's a bit awkward too.

[kleisauke]

The reason I removed the exception that was thrown from get() and set() was because voperation assumes that it returns a boolean, see: https://github.com/jcupitt/lua-vips/blob/master/src/vips/voperation.lua#L217-L220 https://github.com/jcupitt/lua-vips/blob/master/src/vips/voperation.lua#L231-L234

In retrospect, this change isn't correct, because get() does not check for return type, see: https://github.com/jcupitt/lua-vips/blob/master/src/vips/voperation.lua#L254 https://github.com/jcupitt/lua-vips/blob/master/src/vips/voperation.lua#L262 https://github.com/jcupitt/lua-vips/blob/master/src/vips/voperation.lua#L273

[jcupitt]

Yes, I think get/set need revising to be more consistent.

Shall we merge this, then do a second pass to fix that up?

[kleisauke]

Sure, you may merge this. I've tried to fix the get/set on a different branch, see commit: https://github.com/kleisauke/lua-vips/commit/2211a4dc36e496a5ae6572bb0318bd49850a15de

Let me know if that looks better.

By the way, I saw that this repo is missing Travis CI. If you want, I could create a new PR to integrate Travis CI into this repo.

[jcupitt]

Sure, Travis would be great!

I'll have a look at your other branch.

[kleisauke]

Thanks for reviewing and merging this PR!

I've just made a PR for Travis CI integration, see: https://github.com/jcupitt/lua-vips/pull/26.