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

Update RGBA/Rectangle FromValueOptional impls for newly added lifetim… #157

Merged
merged 1 commit into from May 3, 2017

Conversation

Projects
None yet
3 participants
@sdroege
Member

sdroege commented May 3, 2017

…e parameter

See gtk-rs/glib#162
As a next step, these two impls can actually benefit from being there for &Rectangle and &RGBA too to prevent some copying. I'll add those later.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 3, 2017

Member

What would be the correct way to implement this for e.g. &RGBA? I could just do (with NULL checks, etc):

&*(gobject_ffi::g_value_get_boxed(value.to_glib_none_mut().0) as *const RGBA)

But I would expect that there is some infrastructure for that already? FromGlibPtrBorrow sounds like what I need, but I'm not sure how that is supposed to work.
Same story for Rectangle, which also does not use the normal glib_boxed_wrapper macro, which would implement the above trait.

Should this be the following?

impl<'a> FromGlibPtrBorrow<*mut gdk_ffi::RGBA> for &'a RGBA {
    unsafe fn from_glib_borrow(ptr: *mut gdk_ffi::RGBA) -> Self {
        the_code_from_above
    }
}
Member

sdroege commented May 3, 2017

What would be the correct way to implement this for e.g. &RGBA? I could just do (with NULL checks, etc):

&*(gobject_ffi::g_value_get_boxed(value.to_glib_none_mut().0) as *const RGBA)

But I would expect that there is some infrastructure for that already? FromGlibPtrBorrow sounds like what I need, but I'm not sure how that is supposed to work.
Same story for Rectangle, which also does not use the normal glib_boxed_wrapper macro, which would implement the above trait.

Should this be the following?

impl<'a> FromGlibPtrBorrow<*mut gdk_ffi::RGBA> for &'a RGBA {
    unsafe fn from_glib_borrow(ptr: *mut gdk_ffi::RGBA) -> Self {
        the_code_from_above
    }
}
@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

@sdroege Sorry I don't understand what you want to do. Don't see any link between GValue and FromGlibPtrBorrow.
Current version won't work?

Member

EPashkin commented May 3, 2017

@sdroege Sorry I don't understand what you want to do. Don't see any link between GValue and FromGlibPtrBorrow.
Current version won't work?

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 3, 2017

Member

Current version works, but it will always copy the Rectangle/RGBA. I guess not really important for these cases, but it could also just return a (immutable) reference to them as stored inside the Value.

Question is if there is some infrastructure somewhere to convert a *const gdk_ffi::RGBA to a &gdk::RGBA, or if there should just be the cast as above. FromGlibPtrBorrow sounds like it would do something like that, but I don't fully understand how that's supposed to work.

Member

sdroege commented May 3, 2017

Current version works, but it will always copy the Rectangle/RGBA. I guess not really important for these cases, but it could also just return a (immutable) reference to them as stored inside the Value.

Question is if there is some infrastructure somewhere to convert a *const gdk_ffi::RGBA to a &gdk::RGBA, or if there should just be the cast as above. FromGlibPtrBorrow sounds like it would do something like that, but I don't fully understand how that's supposed to work.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

You mean something like this?

impl<'a> glib::value::FromValueOptional<'a> for &'a RGBA {
    unsafe fn from_value_optional(value: &'a glib::Value) -> Option<Self> {
        let ptr = gobject_ffi::g_value_get_boxed(value.to_glib_none().0) as *mut RGBA;
        if ptr.is_null() { None } else { Some(&*ptr) }
}

It compiles, but not sure that it don't conflict with default implementation and don't cause memory problem

Member

EPashkin commented May 3, 2017

You mean something like this?

impl<'a> glib::value::FromValueOptional<'a> for &'a RGBA {
    unsafe fn from_value_optional(value: &'a glib::Value) -> Option<Self> {
        let ptr = gobject_ffi::g_value_get_boxed(value.to_glib_none().0) as *mut RGBA;
        if ptr.is_null() { None } else { Some(&*ptr) }
}

It compiles, but not sure that it don't conflict with default implementation and don't cause memory problem

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

FromGlibPtrBorrow intended to use in signal trampolines for removing unneeded copy or taking ownership. Simply structures like RGBA IMHO always need copy.

Member

EPashkin commented May 3, 2017

FromGlibPtrBorrow intended to use in signal trampolines for removing unneeded copy or taking ownership. Simply structures like RGBA IMHO always need copy.

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 3, 2017

Member

They don't need to be copied, but it probably doesn't hurt much to always copy these structs.

The above compiles and works and was exactly what I meant (and is safe to do), but I wasn't sure if there is infrastructure for such casts already. And FromGlibPtrBorrow sounded like it could do exactly that.

Member

sdroege commented May 3, 2017

They don't need to be copied, but it probably doesn't hurt much to always copy these structs.

The above compiles and works and was exactly what I meant (and is safe to do), but I wasn't sure if there is infrastructure for such casts already. And FromGlibPtrBorrow sounded like it could do exactly that.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member
impl<'a> FromGlibPtrBorrow<*const gobject_ffi::GValue> for &'a RGBA {
    unsafe fn from_glib_borrow(value: *const gobject_ffi::GValue) -> Self {
        let ptr = gobject_ffi::g_value_get_boxed(value) as *mut RGBA;
        &*ptr
    }
}

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

Member

EPashkin commented May 3, 2017

impl<'a> FromGlibPtrBorrow<*const gobject_ffi::GValue> for &'a RGBA {
    unsafe fn from_glib_borrow(value: *const gobject_ffi::GValue) -> Self {
        let ptr = gobject_ffi::g_value_get_boxed(value) as *mut RGBA;
        &*ptr
    }
}

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin

EPashkin May 3, 2017

Member

@GuillaumeGomez restart CI here please

Member

EPashkin commented May 3, 2017

@GuillaumeGomez restart CI here please

@sdroege

This comment has been minimized.

Show comment
Hide comment
@sdroege

sdroege May 3, 2017

Member

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

Why not? I'll take a look at that tomorrow, but in any case not too important for these types :)

Member

sdroege commented May 3, 2017

Can exist too, but it right variant for Option<&'a RGBA> can't 😭

Why not? I'll take a look at that tomorrow, but in any case not too important for these types :)

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

Restarted.

Member

GuillaumeGomez commented May 3, 2017

Restarted.

@EPashkin

This comment has been minimized.

Show comment
Hide comment
@EPashkin
Member

EPashkin commented May 3, 2017

@GuillaumeGomez CI passed

@GuillaumeGomez

This comment has been minimized.

Show comment
Hide comment
@GuillaumeGomez

GuillaumeGomez May 3, 2017

Member

Thanks to both of you!

Member

GuillaumeGomez commented May 3, 2017

Thanks to both of you!

@GuillaumeGomez GuillaumeGomez merged commit 423594c into gtk-rs:master May 3, 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