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

Export GDK_KEY constants as u32. #105

Merged
merged 1 commit into from Mar 3, 2016

Conversation

Projects
None yet
3 participants
@rillian
Contributor

rillian commented Mar 2, 2016

Key values are generally guint or guint32 in API calls,
but gir defines the GDK_KEY_* contants as gint, forcing
every use to be cast. This is probably just the generator
assuming gint for #define contants, and the signed vs.
unsigned distinction doesn't matter in C or most scripting
languages. But it does matter in rust.

Special-casing these in the the gir generator, or marking
up the gdk C header to add a U literal specifier everywhere
is the ideal fix, but in the meantime, we can hide the
bug by casting when we export the ffi version of the constants.

Export GDK_KEY constants as u32.
Key values are generally guint or guint32 in API calls,
but gir defines the GDK_KEY_* contants as gint, forcing
every use to be cast. This is probably just the generator
assuming gint for #define contants, and the signed vs.
unsigned distinction doesn't matter in C or most scripting
languages. But it does matter in rust.

Special-casing these in the the gir generator, or marking
up the gdk C header to add a U literal specifier everywhere
is the ideal fix, but in the meantime, we can hide the
bug by casting when we export the ffi version of the constants.
@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 2, 2016

Member

Good analysis! Can we take this further though? Is there value in keeping the type u32 rather than making an enum?

Member

gkoz commented Mar 2, 2016

Good analysis! Can we take this further though? Is there value in keeping the type u32 rather than making an enum?

@gkoz gkoz referenced this pull request Mar 2, 2016

Closed

Event accessors #108

@rillian

This comment has been minimized.

Show comment
Hide comment
@rillian

rillian Mar 3, 2016

Contributor

An enum would be more natural for rust, but I don't know how to handle keysym aliasing. That is, about 2000 GDK_KEY contants reference the same value as another constant. Rust enums don't allow this. E.g.

/usr/include/gtk-3.0/gdk/gdkkeysyms.h:#define GDK_KEY_Armenian_question 0x100055e
/usr/include/gtk-3.0/gdk/gdkkeysyms.h:#define GDK_KEY_Armenian_paruyk 0x100055e
Contributor

rillian commented Mar 3, 2016

An enum would be more natural for rust, but I don't know how to handle keysym aliasing. That is, about 2000 GDK_KEY contants reference the same value as another constant. Rust enums don't allow this. E.g.

/usr/include/gtk-3.0/gdk/gdkkeysyms.h:#define GDK_KEY_Armenian_question 0x100055e
/usr/include/gtk-3.0/gdk/gdkkeysyms.h:#define GDK_KEY_Armenian_paruyk 0x100055e
@gkoz

This comment has been minimized.

Show comment
Hide comment
@gkoz

gkoz Mar 3, 2016

Member

Okay, looks like aliasing and non-exhaustiveness (there're likely to be values in the wild without corresponding constants) make an enum unsuitable.

Let's merge this then. @GuillaumeGomez

Member

gkoz commented Mar 3, 2016

Okay, looks like aliasing and non-exhaustiveness (there're likely to be values in the wild without corresponding constants) make an enum unsuitable.

Let's merge this then. @GuillaumeGomez

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Mar 3, 2016

Member

@gkoz: I did an enum in the old API but I had then to create an interface function to convert from rust to C, so the current situation is better I think (a bit annoying but we can't do much about it...).

Thanks @rillian!

Member

GuillaumeGomez commented Mar 3, 2016

@gkoz: I did an enum in the old API but I had then to create an interface function to convert from rust to C, so the current situation is better I think (a bit annoying but we can't do much about it...).

Thanks @rillian!

GuillaumeGomez added a commit that referenced this pull request Mar 3, 2016

Merge pull request #105 from rillian/keyval-u32
Export GDK_KEY constants as u32.

@GuillaumeGomez GuillaumeGomez merged commit 9bc1790 into gtk-rs:master Mar 3, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment