-
-
Notifications
You must be signed in to change notification settings - Fork 711
enumtypes: skip generation of "last" members #4520
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enumtypes: skip generation of "last" members #4520
Conversation
I suppose we could also just remove the 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. |
I think removing the |
I checked NetVips, php-vips, pyvips, and ruby-vips, none of these language bindings appear to expose these My only concern is the potential $ vipsthumbnail zebra.jpg --smartcrop=last
Segmentation fault (core dumped) due to: libvips/libvips/conversion/smartcrop.c Line 394 in d87f9ed
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 |
As an experiment, here's a branch that removes these |
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? |
You're right. I just pushed commit kleisauke@1f0054e 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: libvips/libvips/iofuncs/image.c Line 670 in 9f590ce
which is likely safe, I don't expect any new band formats in the near future. |
These `_LAST` enums can be confusing to reference.
This is ready for review. I think removing the |
@@ -174,8 +174,8 @@ typedef enum { | |||
* See also: [method@Image.complex2]. | |||
*/ | |||
typedef enum { | |||
VIPS_OPERATION_COMPLEX2_CROSS_PHASE, | |||
VIPS_OPERATION_COMPLEX2_LAST | |||
VIPS_OPERATION_COMPLEX2_CROSS_PHASE, /*< nick=cross-phase >*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I didn't know you could set nick here, nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. Previously, the nickname was derived by stripping common prefix words from all enum values. However, with only one enum value remaining, this would reduce the nickname to just phase
. This change prevents that.
Great! This is a good cleanup. |
We probably don't want to expose these "last" members to language bindings.
Generated
enumtypes.c
diff: https://gist.github.com/kleisauke/97d3e7d01df0a79b4b9179ea201e053eSee also: libvips/pyvips#537.