Re: Introducing "toggle references"

[Tim Janik <timj gtk org>]

On Wed, 27 Apr 2005, Owen Taylor wrote:

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

it does, every arithmetic has to be performed on types at least
as big as integers.

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

i find it hard to believe that one can write correct bit mangling code 
without knowing about integer promotion...
and adding unneccesary casts is asking for trouble sine it hinders
the compiler from catching mistakes for you.

since you again wrote !=0, you understood me where i said that the
logic is wrong, right?

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

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

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

yeah, i was thinking of GTK_WIDGET_[UN]SET_FLAGS() actually when
i ran into this. i think the API established there is good and
worth copying.

> Regards,
> 						Owen

---
ciaoTJ