On Thu, 2005-04-28 at 01:10 +0200, Tim Janik wrote:
> > 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.
[ ... reads through the standard .. ]
Well, that's a gross simplification, but yes, it behaves as you
describe.
> > 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...
Find it hard to believe you might...
I'll strip the casts down to the minimum in my next version.
> 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?
Yes, I understood, and said as much.
> >>> @@ -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 */
?
> >>> +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.
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.
> > 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.
I'll do a new version tomorrow.
Regards,
Owen
Attachment:
signature.asc
Description: This is a digitally signed message part