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
