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. Cheers, -g -- Greg Stein, http://www.lyra.org/
