Re: GtkBindingSignal changes

[Matthias Clasen <mclasen redhat com>]

On Wed, 2006-01-04 at 13:28 +0100, Tim Janik wrote:
> hi matthias,
> 
> can you please outline the rationale for this change:
> 
> 2005-12-27  Matthias Clasen  <mclasen redhat com>
> 
>          * gtk/gtkbindings.h (GtkBindingSignal):
>          * gtk/gtkbindings.c (binding_signal_new): Make the
>          args a flexible array inside the struct, and allocate them
>          together.
> 
> i think this needs to be backed out because:
> 
> 1) this is an incompatible ABI change (gtkbindings.h):
>     -  GtkBindingArg          *args;
>     +  GtkBindingArg          args[1]; /* flexible array */
> 

Hmm, so people are expected to actually make use of the fact that the
internals of GtkBindingSet are declared in the header ? What for ?

> 2) this code will blow up for n_args = 0 (gtkbindings.c):
>     -  signal = g_new (GtkBindingSignal, 1);
>     +  signal = (GtkBindingSignal *) g_malloc0 (sizeof (GtkBindingSignal) + (n_args - 1) * sizeof (GtkBindingArg));
>        signal->next = NULL;
>     -  signal->signal_name = g_intern_string (signal_name);
>     +  signal->signal_name = (gchar *)g_intern_string (signal_name);
>        signal->n_args = n_args;
>     -  signal->args = g_new0 (GtkBindingArg, n_args);

Yes, that was stupid. James explained what was intended here. The idea
was that if n_args is 0, there is no need to waste any space on args. 

> 3) the code is actually more memory inefficient now.
>     the main use case are signal bindings with 0 args, which used up
>     a single ->args=NULL (4bytes) pointer in GtkBindingSignal.
>     with your change, sizeof (GtkBindingArg)==12 will be used instead.
> 

The numbers don't support you here. From running testgtk --bench ALL, I
see:

67  binding signals with 0 args
194 binding signals with 1 arg
131 binding signals with 2 args
127 binding signals with 3 args

> it's usually a good idea to look up the author from the license comment and
> if he's available ask for review before comitting buggy "improvements" ;)

Sure. 

I get back to the original author once he reviewed my 2-year-old 
gsignal bugfix. 


Matthias