Created attachment 184046 [details] [review]
Proposed patch

Created attachment 184047 [details] [review]
Add G_DESTROY_NOTIFY_CALLBACK

Created attachment 184048 [details] [review]
Use G_DESTROY_NOTIFY_CALLBACK

Comment 1 has the proposed core G_DEFINE_CALLBACK patch. Comment 2 and comment 3 shows how it can be used to improve type-safety for GDestroyNotify. Need feedback before continuing to convert other callback types... Thanks.

In general I like the idea but this will create many new compiler warnings everywhere, right? For example if a callback requires a GObject* as parameter and you pass a GtkWidget* you now have to add a cast while before everything worked without compiler warnings.

Well, this is optional, you have to ask for the pain...

(In reply to comment #5)
> In general I like the idea but this will create many new compiler warnings
> everywhere, right? For example if a callback requires a GObject* as parameter
> and you pass a GtkWidget* you now have to add a cast while before everything
> worked without compiler warnings.

This is probably a style thing ... I mean, if the callback type requires a GObject, then I think it's a bug if your callback expects a GtkWidget. For the same reasons that's why you usually see (at least in the GLib sources)

 static void
 foo_bar_finalize(GObject *object)
 {
   FooBar *bar = FOO_BAR (object);
 [...]

 static void
 foo_bar_class_init (FooBarClass *klass)
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
   klass->finalize = foo_bar_finalize;

and not

 static void
 foo_bar_finalize(FooBar *bar)
 {
 [...]

 static void
 foo_bar_class_init (FooBarClass *klass)
 {
   GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
   klass->finalize = (GObjectFinalizeFunc) foo_bar_finalize;

though there is _some_ code doing the latter (I think some old Nautilus code). Anyway, it's a style thing and a trade-off.

Either way, in the latter case, you can always just choose _not_ to use this new facility by using the existing G_CALLBACK macro or casting yourself. I mean - just add this facility these will not cause compiler warnings - you will have to start using it before you see warnings.

Btw, you can also argue that having something like what I'm proposing will make everybody use the former style... and I actually think that's a good thing - e.g. making people write code in the same style is generally a Good Thing(tm).

(repasting what was lost in the outage)

--- Comment #8 from Bastien Nocera <hadess@hadess.net> 2011-03-24 10:59:03 UTC ---
That means declaring the callback twice (maybe even 3 times). The callback's
arguments are already defined in the class struct, and once more when creating
the marshaller. There should be a way to do this that doesn't involve even more
boilerplate.

--- Comment #9 from David Zeuthen <zeuthen@gmail.com> 2011-03-24 13:45:40 UTC ---
Hey Bastien,

thanks for taking a look at this!

(In reply to comment #8)
> That means declaring the callback twice (maybe even 3 times). The callback's
> arguments are already defined in the class struct, and once more when creating
> the marshaller. There should be a way to do this that doesn't involve even more
> boilerplate.

Note that the proposed G_DEFINE_CALLBACK() macro actually has nothing to do
with the signals as defined in GObject except that the docs mention signals as
an example. In fact, the first usage patch for this isn't for signals - it's
for GDestroyNotify.

But, sure, it would be nice if you didn't have to

 - declare the signal using g_signal_new()
 - add a function pointer to the class struct
 - define a type-safe callback macro for handlers connected via
  g_signal_connect()

but I don't see a way to do all this with the C we are currently allowed to use
in the GLib codebase.

Of course, if someone figures this out we can add this and port over codebases
the same way we've been porting over codebases to use the G_DEFINE_INTERFACE()
macro (which was added in 2.24).

--- Comment #10 from Ryan Lortie (desrt) <desrt@desrt.ca> 2011-03-24 14:44:30 UTC ---
my two cents on this after talking to David on IRC:

i don't like it.

if we want to have typesafe callbacks then i think we should start from the
basis of having a separate connect function for each signal.  of course, this
doesn't really reduce the amount of typing you need to do either:

 - vtable function (no user_data)
 - handler callback type (with user_data)
 - g signal gtypes and marshaller crap (which we could talk about removing)
 - maybe also an emitter

but it presents a somewhat reasonable API to the users of your object:

 gtk_button_clicked_connect (button, a_callback, user_data);

and has the benefit of removing the possibility of typos or thinkos in signal
name strings (although David's proposal also helps a bit here due to its
redundancy).

(In reply to comment #8)
> 
> --- Comment #8 from Bastien Nocera <hadess@hadess.net> 2011-03-24 10:59:03 UTC
> ---
> That means declaring the callback twice (maybe even 3 times). The callback's
> arguments are already defined in the class struct, and once more when creating
> the marshaller. There should be a way to do this that doesn't involve even more
> boilerplate.

If we switched to an IDL...

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/398.