Mailing-List: contact dev-help@apr.apache.org; run by ezmlm
Precedence: bulk
Date: Mon, 3 Sep 2001 22:06:50 -0700
From: Aaron Bannert <aaron@clove.org>
To: dev@apr.apache.org
Subject: Re: [PATCH] new lock APIs, rwlocks, condition variables
Message-ID: <20010903220650.E6340@clove.org>
References: <20010903210824.C6340@clove.org>
 <20010904045327.5245E46993@koj.rkbloom.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
User-Agent: Mutt/1.2.5i
In-Reply-To: <20010904045327.5245E46993@koj.rkbloom.net>;
 from rbb@covalent.net on Mon, Sep 03, 2001 at 09:53:26PM -0700

On Mon, Sep 03, 2001 at 09:53:26PM -0700, Ryan Bloom wrote:
> I have a couple of comments.  :-)
> 
> Overall, with just a quick review, +1 for the concept, I have a few issues with
> the implementation.  Most of my problem is the size of the patch, the good
> thing is that this patch is mostly empty declarations so that we keep
> compiling.
> 
> The problem I have, is that this is actually doing two things.  1)  It re-writes the
> locking API.  2)  It introduces a new condition variable API.  Can we get the patch
> split in two?  To make this a bit easier, I would also recommend that the condition
> variable API belongs in it's own file in the locks subdirectory.

I was thinking that we might want to have a rwlock.c and cond.c for
each implementation, but opted to leave it in until reviewed. It should
be easy enough to split it out.


> Adding that API to the thread locks API that is already there is making the code
> harder to read.  If you split it out, then you can re-post the entire patch in 
> two pieces.  One, the current API re-org, which can be generated by just doing
> a CVS diff.  The other the condition variable API, which can be done by just 
> posting the files with the API to the list.

Are you proposing I do something like apr_cond.h and apr_rwlock.h, leaving
apr_thread_mutex_t and apr_proc_mutex_t in apr_lock.h? That may make this
whole process much easier to digest.


> If you can do that, I can review this patch tomorrow or Tuesday, and hopefully
> apply it the same day.  Also, very nice that this doesn't remove any of the
> current API.  That makes this patch very easy to apply, because I don't have
> to worry about breaking things on half our platforms.  :-)


-aaron