On Wed, 27 Apr 2005, Owen Taylor wrote:
On Thu, 2005-04-28 at 01:10 +0200, Tim Janik wrote:
@@ -1482,7 +1489,7 @@ g_object_weak_ref (GObject *object,
if (wstack)
{
i = wstack->n_weak_refs++;
- wstack = g_realloc (wstack, sizeof (*wstack) + sizeof
(wstack->weak_refs[0]) * i);
+ wstack = g_realloc (wstack, sizeof (*wstack) + sizeof
(wstack->weak_refs[0]) * (i - 1));
}
[...]
+ wstack = g_realloc (wstack, sizeof (*wstack) + sizeof (wstack->toggle_refs[0]) * (i - 1));
you just introduced an off-by-one error, please revert this part (also
affects
your copy-pasted version of this code).
Would you mind if I wrote it as:
sizeof (wstack->weak_refs[0]) * (wstate->n_weak_refs - 1)
The code version using i doesn't make it all clear that the
'i == n_weak_refs - 1' from the ++ is supplying the need to subtract
1 because of the declaration of wstate->weak_refs ... it tripped
me up, and is likely to confuse almost anyone reading the code.
i do in fact mind ;) i wrote the code the way it is because i believe
it is best written that way. that is also why i caught the off-by-one
in your patch. i don't think you can write correct housekeeping code
without paying attention to post- vs. pre-increment or structure sizes
and layout. i have had to fix too many off-by-one errors in my and
other peoples code to believe that.
however, i can understand that the code doesn't look to clear to you,
simply because different programmers have different programming habits.
that doesn't make your version more correct though, and i simply
prefer my version not to be changed.
Yes, but someone is going to CVS annotate the toggle refs stuff and I'm
not fond of writing my code in the form of cryptic puzzles
Would you at least mind if I commented the copy I'm adding as
/* Add tstate->n_weak_refs - 1 positions beyond the 1 declared in
* tstate->nrefs */
?
i don't mind you annotating or altering the copy pasted version
of your code.
+g_object_add_toggle_ref (GObject *object,
[...]
+ if (wstack->n_toggle_refs == 1)
+ G_DATALIST_SET_FLAGS(&object->qdata, OBJECT_HAS_TOGGLE_REF_FLAG);
we had a small chat on this on irc and came to the conclusion that this
could(you)/should(me) be wstack->n_toggle_refs >= 1.
No, I don't think I'd agree there. I'm happy to add a comment:
/* If this is the first toggle reference, set a flag so that we
* can quickly determine whether there are any toggle references.
*/
it simply makes me uncomfortable to see two states supposed
to be in sync (n_toggle_refs>0) == (toggle_flag!=0) to be
synced only on the edge of a specific change, rather than
upon all changes, eventhough for the case at hand, the code
is correct. (that's due to personal experience though, i simply
encountered code that enforces state syncs upon all possible
changes to behave more robust and make the intention clearer,
especially wrg future changes.)
Unless you refuse to accept the patch that way, I'll leave it
the way it is now.
i don't refuse. i'm just stating my preference and its reasons here.
Future changes are more likely to add some non-idempotent
action when the first toggle reference is added, at which point
defensively repeatedly setting the flag will introduce a bug.
i will definitely not argue on what future changes are more likely,
they are hard to predict by nature ;)
Regards,
Owen
---
ciaoTJ
_______________________________________________
gtk-devel-list mailing list
gtk-devel-list@gnome.org
http://mail.gnome.org/mailman/listinfo/gtk-devel-list
