Re: Introducing "toggle references"

[Tim Janik <timj gtk org>]

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