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

Implement SendValue and make Value not implement Send #256

Merged
merged 2 commits into from Nov 15, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented Nov 15, 2017

This is a wrapper around Value that ensures that only Send types are
stored inside it, and as such also implements Send itself. Value does
not implement Send anymore.

Fixes https://github.com/gtk-rs/glib/issues/253

What is missing now is to let gir emit SendValue instead of value if the context it appears in is Send

Add PhantomData<*const c_void> to the Value struct
This ensures that Value is neither Send nor Sync, which it can't be in
general. It is possible to store non-Send/non-Sync values inside the
Value, which could then be transferred to different threads.

Fixes #253
@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez
Member

GuillaumeGomez commented Nov 15, 2017

👍

Implement SendValue
This is a wrapper around Value that ensures that only Send types are
stored inside it, and as such also implements Send itself. Value does
not implement Send anymore.

Fixes #253
@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Nov 15, 2017

Member

I'm not sure how to best do this in gir, @EPashkin? Basically the Value type should be mapped to SendValue wherever the signal/object is Send. Is there a single place where I could do that? Only option I see right now is to do changes everywhere where signals, trampolines, functions are generated (and then also handle &, &mut, Option<>, Option<&>, Option<&mut_> separately).

Member

sdroege commented Nov 15, 2017

I'm not sure how to best do this in gir, @EPashkin? Basically the Value type should be mapped to SendValue wherever the signal/object is Send. Is there a single place where I could do that? Only option I see right now is to do changes everywhere where signals, trampolines, functions are generated (and then also handle &, &mut, Option<>, Option<&>, Option<&mut_> separately).

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Nov 15, 2017

Member

This shouldn't block merging it btw, it is useful as is already (by making Value non-Send).

Member

sdroege commented Nov 15, 2017

This shouldn't block merging it btw, it is useful as is already (by making Value non-Send).

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege Nov 15, 2017

Member

I guess one could insert a fake type for SendValue into the type library, and in a post-processing step after analysis went through replace every glib::Value with that based on the above conditions. Makes sense?

Member

sdroege commented Nov 15, 2017

I guess one could insert a fake type for SendValue into the type library, and in a post-processing step after analysis went through replace every glib::Value with that based on the above conditions. Makes sense?

/// See the [module documentation](index.html) for more details.
#[repr(C)]
pub struct Value(gobject_ffi::GValue);
pub struct Value(gobject_ffi::GValue, PhantomData<*const c_void>);

This comment has been minimized.

@EPashkin

EPashkin Nov 15, 2017

Member

@sdroege What role does PhantomData?

@EPashkin

EPashkin Nov 15, 2017

Member

@sdroege What role does PhantomData?

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017

Member

See commit message :) It makes the struct non-Send. Without having anything inside the struct that is not Send, the compiler will implicitely derive Send.

@sdroege

sdroege Nov 15, 2017

Member

See commit message :) It makes the struct non-Send. Without having anything inside the struct that is not Send, the compiler will implicitely derive Send.

This comment has been minimized.

@EPashkin

EPashkin Nov 15, 2017

Member

Missed this, Thanks.

@EPashkin

EPashkin Nov 15, 2017

Member

Missed this, Thanks.

This comment has been minimized.

@EPashkin

EPashkin Nov 15, 2017

Member

impl !Sync for T {} still nightly-only, maybe this need TODO?

@EPashkin

EPashkin Nov 15, 2017

Member

impl !Sync for T {} still nightly-only, maybe this need TODO?

This comment has been minimized.

@sdroege

sdroege Nov 15, 2017

Member

Ok

@sdroege

sdroege Nov 15, 2017

Member

Ok

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin Nov 15, 2017

Member

👍

Member

EPashkin commented Nov 15, 2017

👍

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez Nov 15, 2017

Member

Thanks!

Member

GuillaumeGomez commented Nov 15, 2017

Thanks!

@GuillaumeGomez GuillaumeGomez merged commit d805af0 into gtk-rs:master Nov 15, 2017

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