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

Allow nullable callbacks #816

Merged
merged 3 commits into from
Oct 20, 2019

Conversation

GuillaumeGomez
Copy link
Member

An example of generation: gtk-rs/gio#230

Note that the gio PR needs #815

cc @EPashkin @sdroege

@@ -244,12 +244,13 @@ impl Builder {
&bounds,
&bounds_names,
false,
&self.callbacks[callback_pos].bound_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not &trampoline.bound_name, in this for?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check if it's the same. Thanks for the lead!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirm, it's the same!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually only for trampoline. Destroy callbacks have this issue. We need to update their bounds.

@EPashkin
Copy link
Member

"glib::Pid" can be added with this diff, but not sure that it best solution

+++ b/src/codegen/function_body_chunk.rs
@@ -1307,6 +1307,13 @@ fn add_chunk_for_type(
             )));
             true
         }
+        library::Type::Alias(_) if ty_name == "glib::Pid" => {
+            body.push(Chunk::Custom(format!(
+                "let {0} = glib::Pid({0});",
+                par.name
+            )));
+            true
+        },
         library::Type::Alias(ref x) => add_chunk_for_type(env, x.typ, par, body, ty_name, nullable),
         ref x =>

@GuillaumeGomez
Copy link
Member Author

I don't want it like this. I'd prefer use a trait instead.

@EPashkin
Copy link
Member

Agree, also this diff not fix generation of "glib::spawn_async"

@EPashkin
Copy link
Member

Seems cargo fmt is needed

@GuillaumeGomez
Copy link
Member Author

I need to update the PR in any case.

@GuillaumeGomez
Copy link
Member Author

Updated.

@EPashkin
Copy link
Member

Not sure that it better than current version, Ex. in gdk.

+/src/auto/functions.rs
@@74,7 +574,7 @@ pub fn threads_add_idle_full<P: Fn() -> bool + Send + Sync + 'static>(
unsafe extern "C" fn notify_func<P: Fn() -> bool + Send + Sync + 'static>(
        data: glib_sys::gpointer,
    ) {
-        let _callback: Box_<P> = Box_::from_raw(data as *mut _);
+        let _callback: Box_<Fn() + 'static> = Box_::from_raw(data as *mut _);
    }
    let destroy_call3 = Some(notify_func::<P> as _);
    let super_callback0: Box_<P> = function_data;
@@ -630,7 +630,7 @@ pub fn threads_add_timeout_full<P: Fn() -> bool + Send + Sync + 'static>(
    unsafe extern "C" fn notify_func<P: Fn() -> bool + Send + Sync + 'static>(
        data: glib_sys::gpointer,
    ) {
-        let _callback: Box_<P> = Box_::from_raw(data as *mut _);
+        let _callback: Box_<Fn() + 'static> = Box_::from_raw(data as *mut _);
    }
    let destroy_call4 = Some(notify_func::<P> as _);
    let super_callback0: Box_<P> = function_data;
@@ -687,7 +687,7 @@ pub fn threads_add_timeout_seconds_full<P: Fn() -> bool + Send + Sync + 'static>
    unsafe extern "C" fn notify_func<P: Fn() -> bool + Send + Sync + 'static>(
        data: glib_sys::gpointer,
    ) {
-        let _callback: Box_<P> = Box_::from_raw(data as *mut _);
+        let _callback: Box_<Fn() + 'static> = Box_::from_raw(data as *mut _);

in gtk:

+++ b/src/auto/cell_layout.rs
@@ -159,9 +159,7 @@ impl<O: IsA<CellLayout>> CellLayoutExt for O {
             None
         };
         unsafe extern "C" fn destroy_func<P: IsA<CellRenderer>>(data: glib_sys::gpointer) {
-            let _callback: Box_<
-                Option<Box<dyn Fn(&CellLayout, &CellRenderer, &TreeModel, &TreeIter) + 'static>>,
-            > = Box_::from_raw(data as *mut _);
+            let _callback: Box_<Q> = Box_::from_raw(data as *mut _);
         }
         let destroy_call4 = Some(destroy_func::<P> as _);
         let super_callback0: Box_<

@sdroege
Copy link
Member

sdroege commented Jul 22, 2019 via email

@sdroege
Copy link
Member

sdroege commented Jul 22, 2019

+/src/auto/functions.rs
@@74,7 +574,7 @@ pub fn threads_add_idle_full<P: Fn() -> bool + Send + Sync + 'static>(
unsafe extern "C" fn notify_func<P: Fn() -> bool + Send + Sync + 'static>(
        data: glib_sys::gpointer,
    ) {
-        let _callback: Box_<P> = Box_::from_raw(data as *mut _);
+        let _callback: Box_<Fn() + 'static> = Box_::from_raw(data as *mut _);
    }

This changes things partially from static to dynamic dispatch, and will crash. We don't create a trait object, but the box is then interpreted as one here. And let's keep static dispatch please.

+++ b/src/auto/cell_layout.rs
@@ -159,9 +159,7 @@ impl<O: IsA<CellLayout>> CellLayoutExt for O {
             None
         };
         unsafe extern "C" fn destroy_func<P: IsA<CellRenderer>>(data: glib_sys::gpointer) {
-            let _callback: Box_<
-                Option<Box<dyn Fn(&CellLayout, &CellRenderer, &TreeModel, &TreeIter) + 'static>>,
-            > = Box_::from_raw(data as *mut _);
+            let _callback: Box_<Q> = Box_::from_raw(data as *mut _);
         }

This does the exact opposite. Created as trait object but then not interpreted as one.

Something's not right with this change :)

@GuillaumeGomez
Copy link
Member Author

Indeed. It seems like I only made half the changes required...

@sdroege
Copy link
Member

sdroege commented Jul 23, 2019

Indeed. It seems like I only made half the changes required...

It should not have any effect on already generated code though, so you also changed things that do not have to be changed :)

@GuillaumeGomez
Copy link
Member Author

No, some changes had to be done, notably on the destructors. The current code couldn't handle more than one correctly.

@sdroege sdroege mentioned this pull request Aug 12, 2019
43 tasks
@GuillaumeGomez GuillaumeGomez force-pushed the generate-nullable-callbacks branch 5 times, most recently from d894961 to 7ec6a55 Compare October 19, 2019 17:30
@GuillaumeGomez
Copy link
Member Author

Should be ready now. Updated the associated PR as well.

src/analysis/functions.rs Outdated Show resolved Hide resolved
@EPashkin
Copy link
Member

@GuillaumeGomez Thanks, I have only small nit

@sdroege
Copy link
Member

sdroege commented Oct 20, 2019

See comment in the gio PR. Otherwise the generated code looks fine

@EPashkin
Copy link
Member

@GuillaumeGomez Thanks, now wait for CI

@GuillaumeGomez
Copy link
Member Author

CI is green! :)

@EPashkin
Copy link
Member

@GuillaumeGomez Thanks again

@EPashkin EPashkin merged commit c1a207d into gtk-rs:master Oct 20, 2019
@GuillaumeGomez GuillaumeGomez deleted the generate-nullable-callbacks branch October 20, 2019 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants