Created attachment 302969 [details] [review]
gsignal: Only use a built-in marshaller when one is not specified

Review of attachment 302969 [details] [review]:

I don't think the patch does what the commit claims.

(In reply to Matthias Clasen from comment #2)
> Review of attachment 302969 [details] [review] [review]:
> 
> I don't think the patch does what the commit claims.

The va_marshaller is set to a builtin one and not left as NULL. If the caller does not override it (like GtkWidget::draw) then the special behavior would be lost.

As a follow up bug, it should check if the c_marshaller is one of the builtin function and if so use the builtin va_marshaller.

Created attachment 303647 [details] [review]
gsignal: Only use a built-in marshaller when one is not specified v2

Added a test which demonstrates the issue.

ping?

I added a test which shows that this is a real bug.

Review of attachment 303647 [details] [review]:

Hmm, do we want to encourage people to use marshallers with side effects?

There's also the issue in that we're introducing a subtle version dependency;
libraries which use this will need to explicitly depend on 2.54.

I'm not opposed to this but...the draw marshaller hasn't been working
since the generic marshalling bits have been merged (right?) which is
quite a while...

Couldn't we also fix this in GTK+ with e.g.:

diff --git a/gtk/gtkwidget.c b/gtk/gtkwidget.c
index 2ed1038..53ad4b1 100644
--- a/gtk/gtkwidget.c
+++ b/gtk/gtkwidget.c
@@ -6357,9 +6357,11 @@ gtk_widget_draw_internal (GtkWidget *widget,
 
           if (g_signal_has_handler_pending (widget, widget_signals[DRAW], 0, FALSE))
             {
+              cairo_save (cr);
               g_signal_emit (widget, widget_signals[DRAW],
                              0, cr,
                              &result);
+              cairo_restore (cr);
             }
           else if (GTK_WIDGET_GET_CLASS (widget)->draw)
             {

?

(And obviously delete all of the custom marshallers too)

GtkWidget::draw still works correctly, but only because it explicitly sets a va marshaller.

The generic marshaller is still slower than an actual marshaller — and we introduced the va-based marshallers to speed up the real marshallers when used in critical paths.

I'd argue that the only reason why generic marshallers ought to be used (and why they were implemented in the first place) is GDBus, where the cost of emitting a signal is not really relevant, since *nobody* will ever stick a DBus signal emission in a critical path.

So, no, I don't particularly agree with the statement of "delete all the custom marshallers too".

In general, I don't agree with the statement that generic marshallers could have side-effects or special behaviours. If you want special behaviour, then you get to write your own marshaller; relying on GLib's own generic marshaller behaviour is not going to give you much, if at all, except the basic functionality of a function call.

I'd be more interested in documenting this fact, instead of changing the behaviour of GLib.

I'm confused then; is there a known library or application impacted by this?

-- 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/1034.