Review comments on signal generation
@federico asked me to look at some generated code, here are the comments :)
Code in question is
gobject_gen! {
class Signaler {
val: Cell<u32>,
}
impl Signaler {
signal fn value_changed(&self);
signal fn value_changed_to(&self, v: u32);
signal fn gimme_an_int(&self) -> u32;
pub fn set_value(&self, v: u32) {
let private = self.get_priv();
private.val.set(v);
self.emit_value_changed();
self.emit_value_changed_to(v);
}
pub fn get_value(&self) -> u32 {
let private = self.get_priv();
private.val.get()
}
pub fn call_emit_gimme_an_int(&self) -> u32 {
self.emit_gimme_an_int()
}
}
}
pub value_changed: Option<unsafe extern "C" fn(this: *mut SignalerFfi) -> (())>,
I'm not sure this is valid because of the unit return value. Unit is a Rust type, a zero-sized type even. And over FFI boundaries those are not ideal. It should not have any return type at all
Same thing in various other places, and also for unit as parameter
gobject_sys::g_signal_emitv(...)
You could directly use glib::Object::emit
here after adding a variant that directly works on the signal id. Currently there is only the variant with a string, but internally that resolves to the signal id anyway so should be trivial to add.
Let unsafe code, less code to generate for you
-
Always calling
g_type_instance_get_private
is not cheap in general, or is it nowadays? Usually the instance private data is just stored inside the instance struct for direct access -
For
GInitiallyUnowned
subclasses you need to use something like myFloatingGuard
fromgst-plugin-rs
in all the trampolines (for signals, vfuncs, etc). Otherwise you can easily run into reference counting disasters -
Having signals automatically add a default handler that panics seems suboptimal. For action signals, sure. But for non-action signals it should either do nothing or there should be none.
-
You currently don't seem to hook up the default signal handlers in the
g_signal_newv
call -
signaler_get_type
probably needs another assertion at the end to ensure the type is valid and registration did not fail -
pub fn new() -> Signaler {
currently usesfrom_glib_full
. For things inheriting fromGInitiallyUnowned
this has to be afrom_glib_none
instead. You might also just want to useglib::Object::new
here -
SignalerExt
also has those functions with unit returns, ok here but not nice/beautiful. Even(())
instead of just()
:) -
connect_value_changed
and others could useglib::Object::connect
instead to prevent some more unsafe code, and you e.g. wouldn't need to generate the trampolines -
connect_value_changed
and others, and signal registration are using the signal name with underscores. Usually signals in GObject are with dashes instead (same thing for properties) -
You probably want to add a concept like
final
classes that can't be inherited from. Then the trait could go away for example, and thedowncast_unchecked
in the signal trampolines, and everything generally would be a bit smaller -
As all this requires nightly anyway, you can get rid of the callback guards. Nightly (and 1.27) have this already built-in