Re: Introducing "toggle references"

[Tim Janik <timj gtk org>]

On Mon, 25 Apr 2005, Owen Taylor wrote:

> On Tue, 2005-04-26 at 01:17 +0200, Tim Janik wrote:

> OK, new version of the patch attached that fixes both.
>
> Well, what I did for 2) was to add public functions and a
> gdatasetprivate.h with the macros and use the macros from gobject.c.
>
> g_object_ref/unref very typically show up high on profiles and I'm
> reluctant to add another PLT function call in them when it is easily
> avoided.
>
> Premature optimization perhaps, but I think pretty harmless premature
> optimization ... the increase in complexity is not significant.

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.

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

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

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

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

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, and we'll worry about the involved atomic refcounting issues
in the atomic refcounting patch.

> Regards,
> 						Owen

---
ciaoTJ