Re: Introducing "toggle references"

[Tim Janik <timj gtk org>]

On Sat, 30 Apr 2005, Owen Taylor wrote:

> On Sat, 2005-04-30 at 13:56 +0200, Tim Janik wrote:
> > On Thu, 28 Apr 2005, Owen Taylor wrote:
> >
> > > On Wed, 2005-04-27 at 20:07 -0400, Owen Taylor wrote:
> > >
> > > > I'll do a new version tomorrow.
> > >
> > > Here's a new version. I think it addresses all the points that
> > > were discussed.
> > >
> > > Two other changes in the new version:
> > >
> > > - in gdatasetprivate.h gsize instead of gulong is used for
> > >   pointer => int casts. Really we'd want uinptr_t, but
> > >   gsize works on all platforms I know. (gulong apparently
> > >   won't work on win64)
> > >
> > > - The docs for g_object_add_weak_ref() are extended to
> > >   describe the use case in some detail.
> >
> > the patch only contained the gobject/ part of your changes.
> > i'm fine with those but i'd also like to see the final dataset.*
> > changeset ;)
>
> Let's try that again ...

ok, i assume you didn't change the GObject part since the last patch.
so with the dataset partch i only have minor issues:

> +#define G_DATALIST_GET_FLAGS(datalist)                                                 \
> +  ((gsize)*(datalist) & G_DATALIST_FLAGS_MASK)
> +#define G_DATALIST_SET_FLAGS(datalist, flags) G_STMT_START {                           \
> +  *datalist = (GData *)(G_DATALIST_GET_FLAGS(datalist) |                               \
> +                       (flags) |                                                       \
> +                       ((gsize)*(datalist) & ~(gsize)G_DATALIST_FLAGS_MASK));          \
> +} G_STMT_END

why is this not simply (GData*) (flags | (gsize) *(datalist)) ?


> +#define G_DATALIST_UNSET_FLAGS(datalist, flags) G_STMT_START {                         \
> +  *datalist = (GData *)((G_DATALIST_GET_FLAGS(datalist) & ~(flags)) |                  \
> +                       (flags) |                                                       \
> +                       ((gsize)*(datalist) & ~(gsize)G_DATALIST_FLAGS_MASK));          \
> +} G_STMT_END

why is this not simply (GData*) (~flags & (gsize) *(datalist)) ?


> +
> +#define G_DATALIST_GET_POINTER(datalist)                                               \
> +  ((GData *)((gsize)*(datalist) & ~(gsize)G_DATALIST_FLAGS_MASK))
> +#define G_DATALIST_SET_POINTER(datalist,pointer) G_STMT_START {                                \
> +  *(datalist) = (GData *)(G_DATALIST_GET_FLAGS (datalist) |                            \
> +                         (gsize)pointer);                                              \
> +} G_STMT_END

it looks to me like SET() and UNSET() became a little over-engineered with
the recent changes and could be simplified. other than that, i'm fine with
the patch being applied.

> 					Owen

---
ciaoTJ