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

Remove Default impl for SourceId #288

Merged
merged 2 commits into from Jan 25, 2018

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Jan 25, 2018

While 0 is never a valid SourceId, it is also not valid to use it with
any functions that work on SourceIds. It causes assertions at runtime.
The pattern of having a "non-existing" SourceId is better suited by
using an Option, and once NonZero is stabilized this can even be done
without introducing any memory overhead.

Remove Default impl for SourceId
While 0 is never a valid SourceId, it is also not valid to use it with
any functions that work on SourceIds. It causes assertions at runtime.
The pattern of having a "non-existing" SourceId is better suited by
using an Option, and once NonZero is stabilized this can even be done
without introducing any memory overhead.
@EPashkin

This comment has been minimized.

Member

EPashkin commented Jan 25, 2018

Not fully understand your reason and solution, but I not against this change.
Maybe also make inner u32 private, the add assert !=0 to impl FromGlib<u32> for SourceId ?

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jan 25, 2018

The reasoning makes sense for me so 👍

@sdroege

This comment has been minimized.

Member

sdroege commented Jan 25, 2018

Maybe also make inner u32 private, the add assert !=0 to impl FromGlib for SourceId ?

It already is private fortunately :) We could add an assertion in the FromGlib impl, but don't do so for SignalHandlerId and similar things either. Should we? If we ever get 0 from C code, things are probably very broken

@EPashkin

This comment has been minimized.

Member

EPashkin commented Jan 25, 2018

There will problem with SignalHandlerId and https://developer.gnome.org/gobject/stable/gobject-Signals.html#g-signal-connect-data: as I understand it can return 0 on wrong input

@EPashkin

This comment has been minimized.

Member

EPashkin commented Jan 25, 2018

If you think, that assertion is informative, the I not against it too.

@sdroege

This comment has been minimized.

Member

sdroege commented Jan 25, 2018

g_signal_connect returns 0 if you provide an invalid object, or a signal name that does not exist on the object. In the bindings this can't really happen, in Object::connect() this can happen and would return Err already (I hope, I'll check).

Ok, so I check the above and add assertions for these cases to the FromGlib impls.

Assert that SignalHandlerIds/SourceIds are not 0 in the FromGlib impls
0 is an invalid value and only happens on errors, which should be caught
by code before calling FromGlib.
@sdroege

This comment has been minimized.

Member

sdroege commented Jan 25, 2018

Looks all fine

@EPashkin

This comment has been minimized.

Member

EPashkin commented Jan 25, 2018

👍

@GuillaumeGomez

This comment has been minimized.

Member

GuillaumeGomez commented Jan 25, 2018

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit ccee59f into gtk-rs:master Jan 25, 2018

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