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