On Wed, 2005-04-27 at 23:41 +0200, Tim Janik wrote:
> complexity is not my concern here.
> i do have a few more comments on your patch though.
>
> > +g_datalist_set_flags (GData **datalist,
> > + guint flags)
> > +{
> > + g_return_if_fail (datalist != NULL);
> > + g_return_if_fail ((flags & ~(guint)G_DATALIST_FLAGS_MASK) != 0);
>
> the cast to guint is uneccessary and uneccessary casts should be avoided
> like the plague. also i think the logic is wrong here. the way i read your
> assertion, you force non G_DATALIST_FLAGS_MASK bits to be set.
The logic is in fact backwards ...
I don't agree about the cast. The (guint) makes the code independent of
the type of G_DATALIST_FLAGS_MASK
That is:
char mask = G_DATALIST_FLAGS_MASK;
g_return_if_fail ((flags & ~mask) != 0);
doesn't give the right answer.
While the C standard may require G_DATALIST_FLAGS_MASK to evaluate as an
expression of type int or unsigned int, that's not something I'd be
sure of without looking at the C standard, so IMO the cast gives the
reader more confidence in the correctness of the code.
> > @@ -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.
> > +g_object_add_toggle_ref (GObject *object,
> > + GToggleNotify notify,
> > + gpointer data)
> > +{
> > + ToggleRefStack *wstack;
> [...]
>
> the wstack variable in your toggle_ref functions should really be
> called tstack.
Sure.
> > +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.
*/
> > +g_object_remove_toggle_ref (GObject *object,
> [...]
> > + if (wstack->n_toggle_refs == 0)
> > + G_DATALIST_SET_FLAGS(&object->qdata, 0);
> > +
> > + g_object_unref (object);
>
> this code portion assumes, that no other datalist flags besides
> OBJECT_HAS_TOGGLE_REF_FLAG exist. if in a year or two, someone
> else starts using 0x2 of the datalist bits, it'll unintentionally
> be reset by this code portion. what you actually want is a
> G_DATALIST_UNSET_FLAGS (&object->qdata, OBJECT_HAS_TOGGLE_REF_FLAG);
> macro (and consequently g_datalist_unset_flags).
That changes the meaning of SET_FLAGS() as well ...but sure, it's
an OK way to do it. (And matches GTK_WIDGET_SET_FLAGS())
> i didn't see any further problems with this patch, other than that it might
> conflict with the atomic reference counting patch which is still on my
> review list. taking a quick peek at the major issues of the last version
> of that patch, i figured it still needs major fixups/discussion, so will
> probably go through another 2-3 review rounds while this one can quickly
> be finished. so please send me a version with the above issues fixed for
> final review
Will do.
Regards,
Owen
Attachment:
signature.asc
Description: This is a digitally signed message part