Re: plea for review

On Wed, 24 Jan 2001, Tim Janik wrote:

> On Wed, 24 Jan 2001, Alexander Larsson wrote:
> > Before starting to review the actual locking i'd like to state that the
> > following locking primitive is completely bogus:
> > 
> > static gint type_rw_lock = 0;
> > #define	G_READ_LOCK(rw_lock)	{ if (*(rw_lock)!=0) g_error (G_STRLOC": read lock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=1; }
> > #define	G_READ_UNLOCK(rw_lock)	{ if (*(rw_lock)!=1) g_error (G_STRLOC": read unlock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=0; }
> > #define	G_WRITE_LOCK(rw_lock)	{ if (*(rw_lock)!=0) g_error (G_STRLOC": write lock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=2; }
> > #define	G_WRITE_UNLOCK(rw_lock)	{ if (*(rw_lock)!=2) g_error (G_STRLOC": write unlock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=0; }
> > 
> > There are so many problems with it I won't even get started on it. I
> > assume you're gonna replace this later with some real thing?
> heh ;)
> these are debugging primitives that should point out obvious
> things like G_READ_LOCK(); G_READ_LOCK(); use
> static GStaticRWLock type_rw_lock = G_STATIC_RW_LOCK_INIT;
> #define	G_READ_LOCK(rw_lock)	g_static_rw_lock_reader_lock (rw_lock)
> #define	G_READ_UNLOCK(rw_lock)	g_static_rw_lock_reader_unlock (rw_lock)
> #define	G_WRITE_LOCK(rw_lock)	g_static_rw_lock_writer_lock (rw_lock)
> #define	G_WRITE_UNLOCK(rw_lock)	g_static_rw_lock_writer_unlock (rw_lock)
> if you prefer the real thing ;)
> note however that pure testing doesn't do too much good currently,
> as a couple of other places in are still not MT safe,
> so the main purpose in me sending gtype.c out was in fact just
> theoretical proof reading ;)

I suspected so.

I've given it a read now, and i couldn't find much problems. I'm extremely
uncomfortable with dropping the lock and calling external code, but I
suppose if finalizers and initializer aren't completely insane it will

  pnode = lookup_type_node_L (parent_type);
  finfo = type_node_fundamental_info_L (pnode);
  if (!pnode)
might call type_node_fundamental_info_L (NULL), which looks bad.
Calls like g_type_fundamental_branch_last() are fundamentally
non-threadsafe. The value returned may have changed before the caller even
gets it.

Otherwise I found nothing, although I can't claim I understand all the

/ Alex

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]