Re: GtkBindingSignal changes

[Tim Janik <timj imendio com>]

On Wed, 4 Jan 2006, Matthias Clasen wrote:

> 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 ?

erm, isn't that the case with everything declared as non-private
in our public header files? ;)

> What for ?

i don't have a good use case at hand, but that doesn't mean it isn't
used out there. if we change public structures at will, or want to do
so, we definitely should make them private first though.


> > 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

hm, i can't reproduce that with HEAD atm:

$ ./testgtk --bench ALL
Test                 Iters      First      Other
-------------------- ----- ---------- ----------
alpha window             1     1905.4
[...]
cursors                  1      145.7
Segmentation fault

but your numbers do indeed look like an interesting finding, though
it's usually better to look at stats from real apps like gimp or evo.

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

oh come on. just because you didn't get timely review for some patch (i don't
even know which one you bother about here, feel free to send it around again),
you take that as an excuse to never ask for patch review again?

peer review is one of the biggest advantages (if not THE biggest advantage) in
free software development. refusing to make use of it for whatever reason is
in effect asking for buggy patches and inappropriate changes.

for every change one makes to foreign code, one should try to get review,
ideally from the original author of the code. even if the chances for a reply
are small, sending out a patch via email still takes only a couple seconds
and may very well be worth it.
often, peer review by another person can pay off even for code portions
you wrote on your own; it did often enough for me at least - but maybe i'm
just producing way buggier code than you on average ;)

> Matthias

---
ciaoTJ