On Wed, 28 Feb 2001, Greg Stein wrote: > 1) it is hard to tell that "p" and "h" refer to the same bucket. > "p->heap.foo" is more obvious than "h->foo".
Fine by me. > 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". We've more or less standardized on one letter meaning a bucket, though... fixing this would be a massive change. For now, I think we need to stick with one letter for consistency. I'd be just as happy if that letter were 'b' (I don't know where a and e came from, but they're definitely there), or at least a letter mnemonic to the type of bucket it is (as I've done here). > 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(!) Done. > 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) Yeah... I just put that comment in because there are comments just like it throughout the buckets code where there's a possibility of failure that we're not checking for, in case we decide at some point down the road to go back and put in checks for them. <shrug> > That's it for now. I'll re-review when you check it in. Thanks. --Cliff
