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 <[EMAIL PROTECTED]> > > * 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 _______________________________________________ gtk-devel-list mailing list [email protected] http://mail.gnome.org/mailman/listinfo/gtk-devel-list
