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

Remove into bound for user callbacks #715

Merged
merged 6 commits into from Jan 31, 2019

Conversation

Projects
None yet
3 participants
@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jan 29, 2019

Fixes #713.

cc @EPashkin @sdroege

@@ -199,7 +200,9 @@ impl Bounds {
last.bound_type = BoundType::NoWrapper;
last.parameter_name = String::new();
Some(new_one)
Some(new_one)*/

This comment has been minimized.

@sdroege

sdroege Jan 29, 2019

Member

Commenting out all this code does not seem ideal. Best to remove it :)

And it's only used for your callbacks, it's not used for other bounds? I.e. there are no other changes in all the crates?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 29, 2019

Author Member

Yes because of need_is_into_check which can only be true if this is a function type and not a GDestroyNotify one.

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 29, 2019

Not sure that it right way, now we lost Option completely. Ex. gtk::Assistant

+++ b/src/auto/assistant.rs
@@ -86,7 +86,7 @@ pub trait AssistantExt: 'static {
 
     fn set_current_page(&self, page_num: i32);
 
-    fn set_forward_page_func<P: Fn(i32) -> i32 + 'static, Q: Into<Option<P>>>(&self, page_func: Q);
+    fn set_forward_page_func<P: Fn(i32) -> i32 + 'static>(&self, page_func: P);
 
     fn set_page_complete<P: IsA<Widget>>(&self, page: &P, complete: bool);
 
@@ -235,9 +235,9 @@ impl<O: IsA<Assistant>> AssistantExt for O {
         }
     }
 
-    fn set_forward_page_func<P: Fn(i32) -> i32 + 'static, Q: Into<Option<P>>>(&self, page_func: Q) {
+    fn set_forward_page_func<P: Fn(i32) -> i32 + 'static>(&self, page_func: P) {
         let page_func = page_func.into();
-        let page_func_data: Box_<Option<P>> = Box::new(page_func.into());
+        let page_func_data: Box_<Option<P>> = Box::new(page_func);
         unsafe extern "C" fn page_func_func<P: Fn(i32) -> i32 + 'static>(current_page: libc::c_int, data: glib_ffi::gpointer) -> libc::c_int {
             let callback: &Option<P> = &*(data as *mut _);
             let res = if let Some(ref callback) = *callback {
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 29, 2019

Yeah that's too much. It should instead be P: Option<Fn(i32) ...>

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:into-bound branch from 711e402 to 0b424b5 Jan 29, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 29, 2019

Okay, Option is now back!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 29, 2019

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 29, 2019

Thanks, but this case also have problem: old case has was good with just type, your need Some and type

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 29, 2019

Do you have an example so I can improve my code? :)

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 29, 2019

I don't have, tried playground for simplified cases all not good 😢

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

So what do you want me to change in here?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 30, 2019

Don't generate any Option<_> in any way for callbacks, and print a warning for each such function that manual implementations might be necessary because of that.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

I'm almost done here. Need to find out why some Into bounds are removed and then we're good.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:into-bound branch from c8772c3 to e49b44d Jan 30, 2019

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:into-bound branch from e49b44d to 59cb650 Jan 30, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

Ok, here we go!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

It generates stuff like:

    fn set_forward_page_func(&self, page_func: Option<&( dyn Fn(i32) -> i32 + 'static)>) {
	let page_func = page_func.into();
        let page_func_data: Box_<Option<&( dyn Fn(i32) -> i32 + 'static)>> = Box::new(page_func);
	unsafe extern "C" fn page_func_func(current_page: libc::c_int, data: glib_ffi::gpointer) -> libc::c_int {
            let callback: &Option<&( dyn Fn(i32) -> i32 + 'static)> = &*(data as *mut _);
            let res = if let Some(ref callback) = *callback {
                callback(current_page)
            } else {
		panic!("cannot get closure...")
            };
            res
        }
        let page_func = if page_func_data.is_some() { Some(page_func_func::<> as _) } else { None };
        unsafe extern "C" fn destroy_func(data: glib_ffi::gpointer) {
            let _callback: Box_<Option<&( dyn Fn(i32) -> i32 + 'static)>> = Box_::from_raw(data as *mut _);
        }
        let destroy_call3 = Some(destroy_func::<> as _);
	let super_callback0: Box_<Option<&( dyn Fn(i32) -> i32 + 'static)>> = page_func_data;
        unsafe {
            ffi::gtk_assistant_set_forward_page_func(self.as_ref().to_glib_none().0, page_func, Box::into_raw(super_\
callback0) as *mut _, destroy_call3);
	}
    }
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 30, 2019

fn set_forward_page_func(&self, page_func: Option<&( dyn Fn(i32) -> i32 + 'static)>) {

Why the parenthesis and the space before the dyn? That should both not be needed :)

Also this one should take a Box<dyn Fn ...>, it's not scope call. This is going to crash in interesting ways :)

scope notified should use boxed trait objects, scope call should use Option<&mut dyn FnMut...>


Once that is fixed, can you paste one example for scope notified and another for scope call?

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 30, 2019

Just to be clear, the parameter for scope notified must be Option<Box<dyn Fn...>>. You only box that box a second time inside the function for making it 1 pointers big instead of 2.

For scope call you don't have any box anywhere, but the parameter is func: Option<&mut dyn Fn...> and you then have to pass &mut func as *mut _ to the C function and in the trampoline you'll get out a *mut *mut dyn Fn (note the double indirection: this is the same as the double boxing in the other case but without heap allocation!).

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

Sure!

GuillaumeGomez added some commits Jan 30, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 30, 2019

notified:

fn set_forward_page_func(&self, page_func: Option<Box<dyn Fn(i32) -> i32 + 'static>>) {
    let page_func_data: Box_<Option<Box<dyn Fn(i32) -> i32 + 'static>>> = Box::new(page_func);
    unsafe extern "C" fn page_func_func(current_page: libc::c_int, data: glib_ffi::gpointer) -> \
libc::c_int {
        let callback: &Option<Box<dyn Fn(i32) -> i32 + 'static>> = &*(data as *mut _);
        let res = if let Some(ref callback) = *callback {
            callback(current_page)
        } else {
            panic!("cannot get closure...")
        };
        res
    }
    let page_func = if page_func_data.is_some() { Some(page_func_func::<> as _) } else { None };
    unsafe extern "C" fn destroy_func(data: glib_ffi::gpointer) {
        let _callback: Box_<Option<Box<dyn Fn(i32) -> i32 + 'static>>> = Box_::from_raw(data as \
*mut _);
    }
    let destroy_call3 = Some(destroy_func::<> as _);
    let super_callback0: Box_<Option<Box<dyn Fn(i32) -> i32 + 'static>>> = page_func_data;
    unsafe {
        ffi::gtk_assistant_set_forward_page_func(self.as_ref().to_glib_none().0, page_func, Box:\
:into_raw(super_callback0) as *mut _, destroy_call3);
    }
}

call:

fn invalidate_maybe_recurse(&self, region: &cairo::Region, child_func: Option<&mut dyn (FnMut(&W\
indow) -> bool)>) {
    let child_func_data: Option<&mut dyn (FnMut(&Window) -> bool)> = child_func;
    unsafe extern "C" fn child_func_func(window: *mut ffi::GdkWindow, user_data: glib_ffi::gpoin\
ter) -> glib_ffi::gboolean {
        let window = from_glib_borrow(window);
        let callback: *mut Option<&mut dyn (FnMut(&Window) -> bool)> = user_data as *const _ as \
usize as *mut Option<&mut dyn (FnMut(&Window) -> bool)>;
        let res = if let Some(ref mut callback) = *callback {
            callback(&window)
        } else {
            panic!("cannot get closure...")
        };
        res.to_glib()
    }
    let child_func = Some(child_func_func::<> as _);
    let super_callback0: &Option<&mut dyn (FnMut(&Window) -> bool)> = &child_func_data;
    unsafe {
        ffi::gdk_window_invalidate_maybe_recurse(self.as_ref().to_glib_none().0, region.to_glib_\
none().0, child_func, super_callback0 as *const _ as usize as *mut _);
    }
}
@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 31, 2019

page_func_func::<>

I didn't know this even compiles :) But fine with keeping it if it doesn't break the compiler.

Looks good to me otherwise.

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

Me neither! I'll clean that up!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

Updated!

@sdroege

This comment has been minimized.

Copy link
Member

sdroege commented Jan 31, 2019

All good for me then

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 31, 2019

Generated result works good, thanks.

Clippy nags again (warnings clippy::collapsible_if IMHO don't need fix now)

error: this `if` has identical blocks
   --> src/codegen/function_body_chunk.rs:267:31
    |
267 |           } else if !is_destroy {
    |  _______________________________^
268 | |             if full_type.is_none() {
269 | |                 if trampoline.scope.is_call() {
270 | |                     chunks.push(
...   |
290 | |             }
291 | |         }
    | |_________^
    |
    = note: #[deny(clippy::if_same_then_else)] on by default
note: same as this
   --> src/codegen/function_body_chunk.rs:243:48
@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

I made it voluntarily but I can change it if you prefer to group conditions in there?

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 31, 2019

So part && *trampoline.nullable makes no difference?
This is strange, and if it true, then IMHO better removed, even it it was needed before

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

Fine by me!

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

Updated!

@EPashkin

This comment has been minimized.

Copy link
Member

EPashkin commented Jan 31, 2019

@GuillaumeGomez Thanks, lets merge this.

@EPashkin EPashkin merged commit f5d3eab into gtk-rs:master Jan 31, 2019

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:into-bound branch Jan 31, 2019

@GuillaumeGomez

This comment has been minimized.

Copy link
Member Author

GuillaumeGomez commented Jan 31, 2019

🎉 I'll send the PRs this evening (except for gtk which is handled through @sdroege's PR).

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.