[cairo] Review of: [cairo-mutex] Add default implementation for CAIRO_MUTEX_INIT that uses CAIRO_MUTEX_NIL_INITIALIZER. This used to be the implementation for pthread because pthread_mutex_init() is broken. See d48bb4fbe876a93199ba48fcf5f32734fbe18ba9.

[Behdad Esfahbod]

On Mon, 2007-04-23 at 16:07 -0400, Carl Worth wrote:
> > diff-tree 97c197478023ceb5477a203d058eaec2cb18f987 (from 6d2a2dd6d9190c62b209e47c083b7df72e7134fb)
> > Author: Behdad Esfahbod <behdad at behdad.org>
> > Date:   Thu Apr 19 16:26:21 2007 -0400
> >
> >     [cairo-mutex] Add default implementation for CAIRO_MUTEX_INIT
> >     that uses CAIRO_MUTEX_NIL_INITIALIZER.  This used to be the
> >     implementation for pthread because pthread_mutex_init() is
> >     broken.  See d48bb4fbe876a93199ba48fcf5f32734fbe18ba9.
> ...
> > +#ifndef CAIRO_MUTEX_INIT
> > +# define CAIRO_MUTEX_INIT(_mutex) do {				\
> > +    cairo_mutex_t _tmp_mutex = CAIRO_MUTEX_NIL_INITIALIZER;     \
> > +    memcpy ((_mutex), &_tmp_mutex, sizeof (_tmp_mutex));        \
> > +} while (0)
> > +#endif
> 
> This is a bit confused. The original intent of
> CAIRO_MUTEX_NIL_INITIALIZER is that it was something that would be
> used to initialize the mutex of a nil object, (so it had to be able to
> compile, but it didn't have to actually mean anything useful, since it
> would never be looked at).

Right.  I noticed this mild change of semantic later . It's more
complicated.  Such semantic is only assumed if you don't define
CAIRO_MUTEX_INIT.  So I can at least claim that its a backward
compatible change: "if you don't define CAIRO_MUTEX_INIT, we assume 
CAIRO_MUTEX_NIL_INITIALIZER is a useful initializer."  and the reason I
did was to avoid duplicating the initialization when an static one does
work.


> But if we're going to use it to initialize real mutex objects, then
> that's a big semantic change, and its name is all wrong.
> 
> Meanwhile, I think this patch exacerbates a problem that started with
> the big mutex rewrite and continued with addition of things like
> CAIRO_MUTEX_USE_GENERIC_INITIALIZATION.

I think you are being too picky now :).  Maybe
CAIRO_MUTEX_USE_GENERIC_INITIALIZATION should have been named with a
leading underscore, but still, it's just part of the implementation.
Nothing to do with the API.  The reason it was introduced is that
previously cairo-mutex.c was conditioning on !HAVE_PTHREAD to define the
initializer routines, and that made no sense.  I just needed a symbol
name and that's what I chose.

Realistically, the problem the mutex rewrite tried to solve was to make
adding new mutexes easy and I think we have completely achieved that.
True, the implementation went untested on win32, but that's nothing new.
And I think cairo-mutex-private.h is very clean and cute right now.  For
the geek inside me at least.


> The problem is that one used to be able to examine the file defining
> all the mutex stuff, find the block of interest for the relevant
> platform, and directly see everything of interest there.

It's still like that.  If it feel better about it we can move the
platform specific part to a separate header, but IMO that's just gong to
make it a lot more confusing.


> Now, however, it's a quite convoluted mess. After you find that block
> you have to search the whole rest of the file, (which is already a bit
> of an #ifdef nightmare), and realize that if CAIRO_MUTEX_INIT isn't
> defined in the block of interest, then something will get
> substituted. Then if CAIRO_MUTEX_INITIALIZE isn't defined then
> something else will also get substituted.

I don't agree.  First, do we even envision a new platform added in the
following two years?  If not, this discussion is irrelevant.  And adding
a new platform is dead easy now: you just look at the other ones, and
copy and adapt one of them.  Of the four we have now, three look VERY
similar.  The only exception is pthread because it doesn't need static
initialization.

In fact it became so easy that I went on and implemented BeOS (a
platform I've never seen).  It was six lines only:

  typedef BLocker* cairo_mutex_t;

# define CAIRO_MUTEX_LOCK(name) (name)->Lock()
# define CAIRO_MUTEX_UNLOCK(name) (name)->Unlock()
# define CAIRO_MUTEX_INIT(mutex) (*(mutex)) = new BLocker()
# define CAIRO_MUTEX_FINI(mutex) delete (*(mutex))
# define CAIRO_MUTEX_NIL_INITIALIZER NULL


> I'd much rather see any common things defined first and the
> substitutions happen explicitly within the small, platform-specific
> blocks, (so that the reader doesn't have to mentally insert implicit
> substitutions in order to understand what's happening).

They won't be that small if you try to move common things into it.


> Meanwhile, the names of CAIRO_MUTEX_INIT and CAIRO_MUTEX_INITIALIZE
> are far too similar for having such distinct behaviors.

I think the only thing lacking is a 20 line documentation of how to add
a new platform.  I was planning to do that.  How about you let me do
that and decide again if you need to fix anything?

On another note, something that really pissed me off was how
CAIRO_MUTEX_INIT/FINI take pointers while CAIRO_MUTEX_LOCK/UNLOCK take
the mutex itself.  Now, *that* is something worth fixing because that
bites everyone.  See 735be3f09d1d150909305ff3232fda42efcb87bd for
example, and all the users of the API need to acknowledge that
difference.  The only reason I didn't fix it is that at some level it
made some sense, but I think making them all take the mutex instead of a
pointer is the way to go.

> I'll attempt a fix of all of this shortly, (unless someone beats me to
> it).

What kind of fix do you exactly have in mind?

> -Carl
> 
> PS. Behdad, your commit message didn't follow the "one-line summary"
> convention.

Humm.  I try something kinda difference that is compatible with the
"one-line summary".  For example when I write:

    [cairo-mutex] Add default implementation for CAIRO_MUTEX_INIT
    that uses CAIRO_MUTEX_NIL_INITIALIZER.  This used to be the
    implementation for pthread because pthread_mutex_init() is
    broken.  See d48bb4fbe876a93199ba48fcf5f32734fbe18ba9.

or

    [cairo-mutex] Warn if no mutex definition found and let sanity macros err
    about undefined CAIRO_MUTEX macros.

I've tried to break the line such that first line serves as a one-line
summary.  Or did I do something worse?


> PPS. If someone were to fix our cairo-commit email-sending hook to
> send a separate message per commit, that would make review like this
> much easier.

Yeah.  The commit mail is useless for anything more than a couple of
commits.  gitk is a lot easier for reviews.


Thanks for the review.  I wish you have raised this concerns before, but
not much harm if you accept the history clutter :).

-- 
behdad
http://behdad.org/

"Those who would give up Essential Liberty to purchase a little
 Temporary Safety, deserve neither Liberty nor Safety."
        -- Benjamin Franklin, 1759