Re: GtkBindingSignal changes

[Matthias Clasen <mclasen redhat com>]

On Wed, 2006-01-04 at 14:42 +0100, Tim Janik wrote:
> 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.
> 

I think in GTK+, how much private stuff is exposed in headers vs marked
as private vs not shown in headers at all is pretty much a function of
the age of the code. 

Would you as the original author of the binding code agree to label the
contents of GtkBindingSet, GtkBindingEntry and GtkBindingSignal as
private ?

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

Hmm, works for me. 

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

Here are the numbers for the gimp, after opening the prefs dialog and 
one image window:

0     40
1    149
2     94
3    169

and here are some numbers for evo:

0     59
1    328
2    165
3    135

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

Ok, lets get back to being reasonable. I'll send you that patch again,
thanks.

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

Sure, fully agreed with your points on peer review. Maybe we should
encourage it more. There are plenty of patches in bugzilla to train
those patch review skills on...

Matthias