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

Start of user callbacks generation #667

Merged
merged 53 commits into from Jan 22, 2019
Merged

Conversation

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 18, 2018

Fixes #618.

Still heavily under development.

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from e67146e to b0d288c Jan 5, 2019
@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 6, 2019

Ok, the PR is "done". I think that the code can be improved since I discovered a lot of parts of gir with this patch. The whole codegen part of gir should need some rework (not talking about my PR here): it's very complicated to read it and not very nice to use. But that'll be for another time! :)

cc @EPashkin @sdroege

@@ -34,12 +34,12 @@ pub fn generate(
if need_generate_inherent(analysis) {
try!(writeln!(w));
try!(write!(w, "impl {} {{", analysis.name));
for func_analysis in &analysis.constructors() {
for func_analysis in &mut analysis.constructors() {
try!(function::generate(w, env, func_analysis, false, false, 1));

This comment has been minimized.

@EPashkin

EPashkin Jan 6, 2019
Member

I not tried to build this but I don't see change in function::generate declaration: is &mut really needed?

This comment has been minimized.

@GuillaumeGomez

GuillaumeGomez Jan 6, 2019
Author Member

No it's not, you're absolutely right.

@GuillaumeGomez GuillaumeGomez changed the title [WIP] Start of user callbacks generation Start of user callbacks generation Jan 6, 2019
@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from 8bd89a8 to 567d208 Jan 6, 2019
@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

Ok, the PR is "done".

Can you provide some PRs for e.g. gio and gtk for showing the newly generated code?

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

Example of generated change.
My worry that we allow call function with None as callback but then panic on call. Its expected?
Also Box_::into_raw(callback); not always called: Ex. src/auto/cell_area.rs:foreach

+++ b/src/auto/assistant.rs
@@ -233,9 +234,35 @@ impl<O: IsA<Assistant>> AssistantExt for O {
 }
 
-    //fn set_forward_page_func<'a, P: Into<Option<&'a /*Unimplemented*/AssistantPageFunc>>, Q: Into<Option</*Unimplemented*/Fundamental: Pointer>>>(&self, page_func: P, data: Q, destroy: /*Unknown conversion*//*Unimplemented*/DestroyNotify) {
-    //    unsafe { TODO: call ffi::gtk_assistant_set_forward_page_func() }
-    //}
+    fn set_forward_page_func<P: Fn(i32) -> i32 + 'static, Q: Into<Option<P>>, R: Fn() + 'static, S: Into<Option<R>>>(&self, page_func: Q, destroy: S) {
+        let page_func = page_func.into();
+        let page_func_data: Option<P> = page_func.into();
+        unsafe extern "C" fn page_func_func<P: Fn(i32) -> i32 + 'static, R: Fn() + 'static>(current_page: libc::c_int, data: glib_ffi::gpointer) -> libc::c_int {
+            let callback: Box_<Box_<(Option<P>, Option<R>)>> = Box_::from_raw(data as *mut _);
+            let res = if let Some(ref callback) = callback.0 {
+                callback(current_page)
+            } else {
+                panic!("cannot get closure...")
+            };
+            Box_::into_raw(callback);
+            res
+        }
+        let page_func = if page_func_data.is_some() { Some(page_func_func::<P, R> as _) } else { None };
+        let destroy_data: Option<R> = destroy.into();
+        unsafe extern "C" fn destroy_func<P: Fn(i32) -> i32 + 'static, R: Fn() + 'static>(data: glib_ffi::gpointer){
+            let callback: Box_<Box_<(Option<P>, Option<R>)>> = Box_::from_raw(data as *mut _);
+            if let Some(ref callback) = callback.1 {
+                callback()
+            } else {
+                panic!("cannot get closure...")
+            };
+        }
+        let destroy = if destroy_data.is_some() { Some(destroy_func::<P, R> as _) } else { None };
+        let super_callback: Box_<Box_<(Option<P>, Option<R>)>> = Box_::new(Box_::new((page_func_data, destroy_data)));
+        unsafe {
+            ffi::gtk_assistant_set_forward_page_func(self.to_glib_none().0, page_func, Box::into_raw(super_callback) as *mut _, destroy);
+        }
+    }
@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 7, 2019

My worry that we allow call function with None as callback but then panic on call. Its expected?

The important part is that if you don't provide a callback, it's not called:

 let page_func = if page_func_data.is_some() { Some(page_func_func::<P, R> as _) } else { None };
@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

The important part is that if you don't provide a callback, it's not called:

What functions don't require a callback? I don't think we should allow no callback, all C function I can think of will explode without callback and it seem unexpected to do this differently in Rust.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

@GuillaumeGomez Yes, I missed that,
and that P not option actually: line let page_func = page_func.into(); confused me.

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

Example of generated change.

This is for scope async, thanks. How does it look for scope call?

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

  • fn set_forward_page_func<P: Fn(i32) -> i32 + 'static, Q: Into<Option

    >, R: Fn() + 'static, S: Into<Option>>(&self, page_func: Q, destroy: S) {

The destroy notify callback should not be exposed to Rust. It's meant for memory management, and we manage memory automatically in Rust. It is where you would free the closure automatically.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

@sdroege This for scope=notified

<parameter name="page_func" transfer-ownership="none" nullable="1" allow-none="1" scope="notified" closure="1" destroy="2">
<parameter name="destroy" transfer-ownership="none" scope="async">
@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

@sdroege This for scope=notified

Erm, yes. I meant scope notified not scope async (the latter is what we handled before already) :)

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

this gtk_cell_area_foreach with <parameter name="callback" transfer-ownership="none" scope="call" closure="1">

+++ b/src/auto/cell_area.rs
@@ -235,13 +236,45 @@ impl<O: IsA<CellArea>> CellAreaExt for O {
         }
     }
 
-    //fn foreach<P: Into<Option</*Unimplemented*/Fundamental: Pointer>>>(&self, callback: /*Unknown conversion*//*Unimplemented*/CellCallback, callback_data: P) {
-    //    unsafe { TODO: call ffi::gtk_cell_area_foreach() }
-    //}
+    fn foreach<P: Fn(CellRenderer) -> bool + 'static, Q: Into<Option<P>>>(&self, callback: Q) {
+        let callback_data: Box_<Box_<Option<P>>> = Box::new(Box::new(callback.into()));
+        unsafe extern "C" fn callback_func<P: Fn(CellRenderer) -> bool + 'static>(renderer: *mut ffi::GtkCellRenderer, data: glib_ffi::gpointer) -> glib_ffi::gboolean {
+            let renderer = from_glib_none(renderer);
+            let callback: Box_<Box_<Option<P>>> = Box_::from_raw(data as *mut _);
+            let res = if let Some(ref callback) = **callback {
+                callback(renderer)
+            } else {
+                panic!("cannot get closure...")
+            };
+            res.to_glib()
+        }
+        let callback = if callback_data.is_some() { Some(callback_func::<P> as _) } else { None };
+        let super_callback: Box_<Box_<Option<P>>> = callback_data;
+        unsafe {
+            ffi::gtk_cell_area_foreach(self.to_glib_none().0, callback, Box::into_raw(super_callback) as *mut _);
+        }
+    }
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

Seems we need pass NULL as data if callback is None too.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

There many errors on glib,
fixed with #[doc(hidden)]extern crate glib_sys as glib_ffi; but maybe problem really in gir

error[E0433]: failed to resolve: use of undeclared type or module `glib_ffi`
  --> D:/eap/rust/0/glib\src\auto\main_context.rs:62:82
   |
62 |         unsafe extern "C" fn function_func<P: Fn() -> bool + 'static>(user_data: glib_ffi::gpointer) -> glib_ffi::gboolean {
   |                                                                                  ^^^^^^^^ use of undeclared type or module `glib_ffi`
@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

Seems we need pass NULL as data if callback is None too.

Yeah, None as callback is something we should simply not allow :)

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

Yeah, None as callback is something we should simply not allow :)

IMHO it not always, sometimes callback set and then need be removed, so None maybe allowed

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

this gtk_cell_area_foreach with <parameter name="callback" transfer-ownership="none" scope="call" closure="1">

Ok that needs some changes too. It should be similar to this and work on an FnMut reference (not box).

The generated code right now would also crash as it frees the closures on the first call

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

IMHO it not always, sometimes callback set and then need be removed, so None maybe allowed

Do you have an example?

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

IMHO it not always, sometimes callback set and then need be removed, so None maybe allowed

Do you have an example?

And in that case we need an annotation/configuration to distinguish the two cases. But for the normal case, for 99% of the cases, no callback is an error and should not be allowed.

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

IMHO it not always, sometimes callback set and then need be removed, so None maybe allowed

Do you have an example?

And in that case we need an annotation/configuration to distinguish the two cases. But for the normal case, for 99% of the cases, no callback is an error and should not be allowed.

Ah, we can distinguish theses cases by the nullable annotation on the closure. See e.g. gtk_list_box_bind_model().

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 7, 2019

  • let callback: Box_<Box_<(Option

    , Option)>> = Box_::from_raw(data as *mut _);

Here you probably want to instead use a reference instead of from_raw(), i.e. how we do it for signal trampolines already. The problem with from_raw() is that it takes ownership of the pointer, but that's not what happens here.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

Strange, but this PR change sys too, so glib just not compiled.

+++ b/gobject-sys/src/lib.rs
@@ -345,10 +345,10 @@ pub struct GInitiallyUnownedClass {
     pub g_type_class: GTypeClass,
     pub construct_properties: *mut glib::GSList,
     pub constructor: Option<unsafe extern "C" fn(GType, c_uint, *mut GObjectConstructParam) -> *mut GObject>,
-    pub set_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut GValue, *mut GParamSpec)>,
+    pub set_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *const GValue, *mut GParamSpec)>,
     pub get_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut GValue, *mut GParamSpec)>,
     pub dispose: Option<unsafe extern "C" fn(*mut GObject)>,
-    pub finalize: Option<unsafe extern "C" fn(*mut GObject)>,
+    pub finalize: Option<unsafe extern "C" fn(*mut GParamSpec)>,
     pub dispatch_properties_changed: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut *mut GParamSpec)>,
     pub notify: Option<unsafe extern "C" fn(*mut GObject, *mut GParamSpec)>,
     pub constructed: Option<unsafe extern "C" fn(*mut GObject)>,
@@ -396,10 +396,10 @@ pub struct GObjectClass {
     pub g_type_class: GTypeClass,
     pub construct_properties: *mut glib::GSList,
     pub constructor: Option<unsafe extern "C" fn(GType, c_uint, *mut GObjectConstructParam) -> *mut GObject>,
-    pub set_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut GValue, *mut GParamSpec)>,
+    pub set_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *const GValue, *mut GParamSpec)>,
     pub get_property: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut GValue, *mut GParamSpec)>,
     pub dispose: Option<unsafe extern "C" fn(*mut GObject)>,
-    pub finalize: Option<unsafe extern "C" fn(*mut GObject)>,
+    pub finalize: Option<unsafe extern "C" fn(*mut GParamSpec)>,
     pub dispatch_properties_changed: Option<unsafe extern "C" fn(*mut GObject, c_uint, *mut *mut GParamSpec)>,
     pub notify: Option<unsafe extern "C" fn(*mut GObject, *mut GParamSpec)>,
@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 7, 2019

and some new clippy warnings, Ex.

warning: The function/method `codegen::function::bounds` doesn't need a mutable reference
   --> src\codegen\properties.rs:116:55
    |
116 |                     bound = codegen::function::bounds(&mut bounds, &[], false, false).0;
    |                                                       ^^^^^^^^^^^
    |
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
@@ -148,6 +149,7 @@ fn field_ffi_type(env: &Env, field: &Field) -> Result {
Ok(signature)
}
} else {
println!("7 field");

This comment has been minimized.

@EPashkin

EPashkin Jan 7, 2019
Member

2 debug prints

@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 7, 2019

So to sum up a bit what I have to do on my gir pr:

  • not allow to give destroy callback
  • not make my changes when sys rendering is running (changing the type of a callback)
  • take nullability into account for callbacks
  • scope call fixes (FnMut, memory stuff)
  • arrays
  • Use gir indices instead of checking on types: scope="call" closure="1" nullable="1" allow-none="1" scope="notified" closure="2" destroy="3"
  • Handle multiple destroy callbacks
@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from 0b8efe0 to f30cc0a Jan 21, 2019
@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 22, 2019

Now that I added missing from_glib_borrow implementations, what is missing here?

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 22, 2019

Now that I added missing from_glib_borrow implementations, what is missing here?

Does analysis only add things to the imports now after it is known that the function can actually be generated? That was the only other thing that was missing from my side.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 22, 2019

Maybe better change all or almost all error! to warn! and add current crate check if possible.
Ex. g_bytes_new_with_free_func: no user data point to the destroy callback` also appears in gdk-pixbuf, gio,
Anyway as examples build IMHO we can merge and fix errors detected on regens separately.

@sdroege
Copy link
Member

@sdroege sdroege commented Jan 22, 2019

Now that I added missing from_glib_borrow implementations, what is missing here?

Does analysis only add things to the imports now after it is known that the function can actually be generated? That was the only other thing that was missing from my side.

Yes that's solved now. All good to go for me

@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 22, 2019

Ok, then I update the error! to warn! and add the filter on the error messages.

@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 22, 2019

Updated.

// Check for cross "user data".
if cross_user_data_check.values().collect::<Vec<_>>().windows(2).any(|a| a[0] == a[1]) {
*commented = true;
warn!("`{}`: Different user data share the same destructors", func.name);

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019
Member

type_tid available, so please use warn_main!

in_trait,
);
} else {
warn!("`{}`: this is supposed to be a callback function but no callback was \

This comment has been minimized.

if success_parameters.is_empty() {
warn!("{}: missing success parameters for async future", func.name);
} else if error_parameters.is_empty() {
warn!("{}: missing error parameters for async future", func.name);

This comment has been minimized.

@EPashkin

EPashkin Jan 22, 2019
Member

Better in 3 here too, but I do not insist as it need type_id pass

@GuillaumeGomez GuillaumeGomez force-pushed the GuillaumeGomez:user-callbacks branch from 6e31444 to 3c365ce Jan 22, 2019
@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 22, 2019

Force-pushed to fix the last comments.

@EPashkin
Copy link
Member

@EPashkin EPashkin commented Jan 22, 2019

@GuillaumeGomez Big thanks.
Lets merge 🎉

@EPashkin EPashkin merged commit 384e41e into gtk-rs:master Jan 22, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@GuillaumeGomez GuillaumeGomez deleted the GuillaumeGomez:user-callbacks branch Jan 22, 2019
@GuillaumeGomez
Copy link
Member Author

@GuillaumeGomez GuillaumeGomez commented Jan 22, 2019

Thanks for the reviews!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants