Skip to content
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

generate from_glib_borrow implementation for const pointers #428

Merged
merged 1 commit into from Jan 22, 2019

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 21, 2019

impl $crate::translate::FromGlibPtrBorrow<*const $ffi_name> for $name {
#[inline]
unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
$name($crate::translate::from_glib_borrow(ptr as usize as *mut $ffi_name))

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

Why the cast via usize? That shouldn't be needed at all, just the cast to a mutable pointer should be needed

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

Also everywhere else

@@ -391,6 +399,17 @@ impl<T: 'static, MM: BoxedMemoryManager<T>> FromGlibPtrBorrow<*mut T> for Boxed<
}
}

impl<T: 'static, MM: BoxedMemoryManager<T>> FromGlibPtrBorrow<*const T> for Boxed<T, MM> {

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

This shouldn't be needed as you always go via the *mut variant anyway

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 21, 2019

Looks good to me otherwise (same thing in all 3 files)

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from 9cda3a1 to fb57f55 Jan 21, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 21, 2019

Like this?

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from fb57f55 to 2f4f7bc Jan 21, 2019

@@ -400,3 +408,15 @@ impl<T: 'static, MM: SharedMemoryManager<T>> FromGlibPtrBorrow<*mut T> for Share
}
}
}

impl<T: 'static, MM: SharedMemoryManager<T>> FromGlibPtrBorrow<*const T> for Shared<T, MM> {

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

Shouldn't be needed

impl $crate::translate::FromGlibPtrBorrow<*const $ffi_name> for $name {
#[inline]
unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
$name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name))

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

I believe you can also do

$crate::translate::from_glib_borrow(ptr as *mut $ffi_name)

here and in the other places. Without the $name(...) part.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

Isn't it what I did?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

Oh nevermind.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from 2f4f7bc to f931368 Jan 21, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 21, 2019

Updated.

impl $crate::translate::FromGlibPtrBorrow<*const $ffi_name> for $name {
#[inline]
unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
$name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name))

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

You can also remove the $name(...) part here :)

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

It's everywhere! :o

unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
debug_assert!($crate::types::instance_of::<Self>(ptr as *const _));
$name($crate::translate::from_glib_borrow(ptr as *mut _),
::std::marker::PhantomData)

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

And here. And the assertion is already in the function you'd call then, and could also be removed

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from f931368 to 43a36e8 Jan 21, 2019

#[inline]
#[cfg_attr(feature = "cargo-clippy", allow(cast_ptr_alignment))]
unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
$crate::translate::from_glib_borrow(ptr as *mut _)

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

This could also be an explicit cast to *mut $ffi_name, but otherwise all good :) Please merge after changing that

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

You order, I execute!

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

No, it needs to be as _.

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

Otherwise we get:

error[E0277]: the trait bound `object::ObjectRef: translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not satisfied
   --> src/object.rs:529:23
    |
529 |                   $name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not implemented for `object::ObjectRef`
    | 
   ::: src/gobject/auto/binding.rs:12:1
    |
12  | / glib_wrapper! {
13  | |     pub struct Binding(Object<ffi::GBinding, BindingClass>);
14  | |
15  | |     match fn {
16  | |         get_type => || ffi::g_binding_get_type(),
17  | |     }
18  | | }
    | |_- in this macro invocation
    |
    = help: the following implementations were found:
              <object::ObjectRef as translate::FromGlibPtrBorrow<*const gobject_ffi::GObject>>
              <object::ObjectRef as translate::FromGlibPtrBorrow<*mut gobject_ffi::GObject>>
note: required by `translate::from_glib_borrow`
   --> src/translate.rs:1052:1
    |
1052| pub unsafe fn from_glib_borrow<P: Ptr, T: FromGlibPtrBorrow<P>>(ptr: P) -> T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0277]: the trait bound `object::ObjectRef: translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not satisfied
   --> src/object.rs:529:23
    |
529 |                   $name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not implemented for `object::ObjectRef`
    | 
   ::: src/gobject/auto/binding.rs:12:1
    |
12  | / glib_wrapper! {
13  | |     pub struct Binding(Object<ffi::GBinding, BindingClass>);
14  | |
15  | |     match fn {
16  | |         get_type => || ffi::g_binding_get_type(),
17  | |     }
18  | | }
    | |_- in this macro invocation
    |
    = help: the following implementations were found:
              <object::ObjectRef as translate::FromGlibPtrBorrow<*const gobject_ffi::GObject>>
              <object::ObjectRef as translate::FromGlibPtrBorrow<*mut gobject_ffi::GObject>>
note: required by `translate::from_glib_borrow`
   --> src/translate.rs:1052:1
    |
1052| pub unsafe fn from_glib_borrow<P: Ptr, T: FromGlibPtrBorrow<P>>(ptr: P) -> T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from 43a36e8 to 0cd7512 Jan 21, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 21, 2019

I removed stuff and updated the $crate_name stuff.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from 0cd7512 to b2ecf1a Jan 21, 2019

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 21, 2019

👍

@@ -526,11 +526,20 @@ macro_rules! glib_object_wrapper {
#[cfg_attr(feature = "cargo-clippy", allow(cast_ptr_alignment))]
unsafe fn from_glib_borrow(ptr: *mut $ffi_name) -> Self {
debug_assert!($crate::types::instance_of::<Self>(ptr as *const _));
$name($crate::translate::from_glib_borrow(ptr as *mut _),
$name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name),

This comment has been minimized.

@EPashkin

EPashkin Jan 21, 2019

Member

ptr: *mut $ffi_name converted to *mut $ffi_name ? seems previous it was something else, IMHO ObjectRef need *mut $crate::gobject_ffi::GObject but not sure .

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

I replaced with as _. Seems better.

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

Why? You need a *mut $ffi_name here and not some random pointer. It's safer to be explicit here

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 21, 2019

Author Member

I'm afraid we might have the same issue than the other similar change but if you're confident we won't, then I just rollback the last force push.

This comment has been minimized.

@EPashkin

EPashkin Jan 21, 2019

Member

from_glib_borrow here used to create ObjectRef and it IMHO need only * mut GObject
from its shared implementation

This comment has been minimized.

@EPashkin

EPashkin Jan 21, 2019

Member

Locally I get this error:

error[E0277]: the trait bound `object::ObjectRef: translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not satisfied
   --> D:/eap/rust/0/glib\src\object.rs:529:23
    |
529 |                   $name($crate::translate::from_glib_borrow(ptr as *mut $ffi_name),
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `translate::FromGlibPtrBorrow<*mut gobject_ffi::GBinding>` is not implemented for `object::ObjectRef`
    | 
   ::: D:/eap/rust/0/glib\src\gobject\auto\binding.rs:12:1
    |
12  | / glib_wrapper! {
13  | |     pub struct Binding(Object<ffi::GBinding, BindingClass>);
14  | |
15  | |     match fn {
16  | |         get_type => || ffi::g_binding_get_type(),
17  | |     }
18  | | }
    | |_- in this macro invocation
    |
    = help: the following implementations were found:
              <object::ObjectRef as translate::FromGlibPtrBorrow<*const gobject_ffi::GObject>>
              <object::ObjectRef as translate::FromGlibPtrBorrow<*mut gobject_ffi::GObject>>
note: required by `translate::from_glib_borrow`
   --> D:/eap/rust/0/glib\src\translate.rs:1052:1
    |
1052| pub unsafe fn from_glib_borrow<P: Ptr, T: FromGlibPtrBorrow<P>>(ptr: P) -> T {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

Yes, that's exactly what I'm saying :) Remove the $name(...) part and it will work

This comment has been minimized.

@EPashkin

EPashkin Jan 21, 2019

Member

Removing name IMHO don't help as this is definition from_glib_borrow(*mut $ffi_name) that used by ::from_glib_borrow

This comment has been minimized.

@EPashkin

EPashkin Jan 21, 2019

Member

*mut $crate::gobject_ffi::GObject IMHO solve problem

This comment has been minimized.

@sdroege

sdroege Jan 21, 2019

Member

We might be talking about different functions. I mean the implementation for *const $ffi_name that should just call into the one for *mut $ffi_name.

The implementation for *mut $ffi_name needs the $name(...) wrapper and has to be cast to *mut GObject.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from b2ecf1a to b1b98b9 Jan 21, 2019

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 21, 2019

Now looks all good to me

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch 3 times, most recently from 6e46421 to 2bf5746 Jan 21, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 21, 2019

We now have this error:

thread 'subclass::object::test::test_signals' has overflowed its stack

I can't figure out where it's coming from for the moment so I'll check tomorrow.

@@ -526,8 +526,16 @@ macro_rules! glib_object_wrapper {
#[cfg_attr(feature = "cargo-clippy", allow(cast_ptr_alignment))]
unsafe fn from_glib_borrow(ptr: *mut $ffi_name) -> Self {
debug_assert!($crate::types::instance_of::<Self>(ptr as *const _));
$name($crate::translate::from_glib_borrow(ptr as *mut _),
::std::marker::PhantomData)
$crate::translate::from_glib_borrow(ptr as *mut $ffi_name)

This comment has been minimized.

@sdroege

sdroege Jan 22, 2019

Member

The stack overflow comes from here. This impl should've stayed as it was. I was only talking about the new impls in my review comments.

What now happens is that this impl here is calling itself again.

#[inline]
#[cfg_attr(feature = "cargo-clippy", allow(cast_ptr_alignment))]
unsafe fn from_glib_borrow(ptr: *const $ffi_name) -> Self {
$crate::translate::from_glib_borrow(ptr as *mut _)

This comment has been minimized.

@sdroege

sdroege Jan 22, 2019

Member

And this one here can get an explicit cast to *mut $ffi_type

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:borrow branch from 2bf5746 to 9dbaa81 Jan 22, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 22, 2019

All green!

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 22, 2019

👍

@GuillaumeGomez GuillaumeGomez merged commit d778e3a into gtk-rs:master Jan 22, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:borrow branch Jan 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.