enumtypes: skip generation of "last" members

[kleisauke]

We probably don't want to expose these "last" members to language bindings.

Generated enumtypes.c diff: https://gist.github.com/kleisauke/97d3e7d01df0a79b4b9179ea201e053e

See also: https://github.com/libvips/pyvips/pull/537.

[jcupitt]

I suppose we could also just remove the _LAST members and do it at runtime instead. We could have some compatibility defines which fetched the n_values field:

https://docs.gtk.org/gobject/struct.EnumClass.html

We wouldn't be able to get the size of the enum statically, but maybe that's not too bad?

ruby-vips, php-vips etc might need changes too.

Maybe that's too big a change.

[kleisauke]

I think removing the _LAST members would cause a lot of issues downstream, e.g. sharp uses to mean "unset", see: https://github.com/lovell/sharp/blob/v0.34.1/src/pipeline.h#L380-L381

[kleisauke]

I checked NetVips, php-vips, pyvips, and ruby-vips, none of these language bindings appear to expose these _LAST enum members. However, you might still be able to pass them as string literals, if supported (NetVips no longer supports this).

My only concern is the potential vips_enum_from_nick() API break, though it would guarantee that these _LAST members aren't used further downstream. For example, this would segfault on the master branch:

$ vipsthumbnail zebra.jpg --smartcrop=last
Segmentation fault (core dumped)

due to: https://github.com/libvips/libvips/blob/d87f9ed45f2de548ed13af4d1291f1fc0da3cec7/libvips/conversion/smartcrop.c#L394

But with this PR, it would error with:

$ vipsthumbnail zebra.jpg --smartcrop=last
vipsthumbnail: unable to thumbnail zebra.jpg
vipsthumbnail: enum 'VipsInteresting' has no member 'last', should be one of: none, centre, entropy, attention, low, high, all

[kleisauke]

As an experiment, here's a branch that removes these _LAST enum members instead: https://github.com/libvips/libvips/compare/master...kleisauke:enums-remove-last-member

[jcupitt]

I think we'd need something for uses like:

    if (mode[i] < 0 ||
        mode[i] >= VIPS_BLEND_MODE_LAST) {

Maybe:

// tiny utility func
GEnumClass *vips__get_enum_class(const char *class_name);

#define VIPS_BLEND_MODE_LAST (vips__get_enum_class("VipsBlendMode")->n_values)

Or perhaps just:

// returns n_values
int vips__get_enum_last(const char *class_name);


    if (mode[i] < 0 ||
        mode[i] > vips__get_enum_last("VipsBlendMode")) {

What do you think?

[kleisauke]

You're right. I just pushed commit https://github.com/kleisauke/libvips/commit/1f0054ec8fac8be5c84f2ef50988bc0ef3bd0d7f to that branch, since that's probably(?) the only place where it needs to be future-proof.

Note that we sometimes do similar checks elsewhere, for example: https://github.com/libvips/libvips/blob/9f590cea0cf4ab9fdef6ef097fb2a8f81f3c8914/libvips/iofuncs/image.c#L670

which is likely safe, I don't expect any new band formats in the near future.

[kleisauke]

This is ready for review. I think removing the _LAST enum members would be too disruptive for now.

[jcupitt]

Great! This is a good cleanup.