apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Stein <gst...@lyra.org>
Subject Re: Bucket API cleanup issues
Date Wed, 28 Feb 2001 22:59:38 GMT
On Wed, Feb 28, 2001 at 03:03:08PM -0500, Cliff Woolley wrote:
> Basically, instead of having the pool bucket contain a pointer to a heap
> bucket and having to fool with manipulating reference counts and keeping
> track of when to destroy the old apr_bucket_pool struct, the
> apr_bucket_pool *contains* the apr_bucket_heap struct that it will become
> as its first element.

Good call!

Some comments on the patch:

1) it is hard to tell that "p" and "h" refer to the same bucket.
   "p->heap.foo" is more obvious than "h->foo".

2) Nit: I'm getting tired of these single letters for the bucket stuff. "a"
   as a variable for a bucket? "e"? Blarg. Using "h" and "p" fall right into
   that camp; especially p, given its use as a pool or char pointer in so
   many other contexts. I might suggest "bh" and "bp".

3) You have two "base" members. That will be confusing as hell over the long
   haul. I'd recommend using just heap.base. Guess how to detect when a pool
   bucket is no longer in a pool? Right in one! Just set pool to NULL. And
   *please* put some comments to this effect in the apr_bucket_pool
   structure declaration(!)

4) And no... I don't think we need to check malloc() results. If NULL is
   returned, then we'll segfault right away. That's fine in my book since
   we'd otherwise just call abort(). (if you really want to check, then call
   abort() on error or a pool's abort function)

That's it for now. I'll re-review when you check it in.


Greg Stein, http://www.lyra.org/

View raw message