httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
Subject Re: [PATCH] buckets: make API a little cleaner
Date Sat, 22 Jul 2000 03:11:38 GMT

I am against this patch.  It just adds a layer that is unnecessary, and it
is going to be taken out by the compiler anyway.  I also dislike name
changes like this one.

Some more details are below.

> Also, while I'm thinking about it, I notice that in the patched
> http_core.c, there are lots of calls to b->insert() right after bucket b
> is created.  Wouldn't it be easier to have parameters to the bucket
> creation function that would create a bucket and populate it at the same
> time?  This seems to make particular sense in the case of bucket colors

Yes, it would make sense.  But, it keeps us from creating a list of empty
buckets that can be re-used.  This is an optimization that we can make
later.  The other option is to have both create functions, but that just
makes the API a bit less clean.

> like mmap and rmem, where the data can only be "insert"ed once anyway. 
> It's possible that in these cases, b->insert could actually be NULL,
> since we really don't want people inserting stuff into these buckets
> manually.  They're read-only; they can only have data stuffed into them
> at the moment they're created... but I won't make that particular change
> just yet.
> So ap_bucket_*_create() would become ap_bucket_*_create(buf, nbyte, w),
> where buf could be NULL if you really do want an empty bucket.  That
> saves a function call each time you make a new bucket and have something
> to put in it right away.  I'm working on that patch.  I'd post it now,
> but in the process of fixing up the calls to b->insert() all over the
> place, I realized that there were some problems with the *_split()
> functions as I mentioned in my previous email.  So I kind of need to fix
> that all in one fell swoop, as they're so interrelated.  Look for that
> patch tomorrow.

> -APR_EXPORT(const char *) ap_get_bucket_char_str(ap_bucket *b)
> +APR_EXPORT(const char *) ap_bucket_getstr(ap_bucket *b)
>  {
> -    if (b) {
> +    if (b && b->getstr) {
>          return b->getstr(b);

The getstr function can never be NULL, so this is an unnecessary check.

> -APR_EXPORT(int) ap_get_bucket_len(ap_bucket *b)
> +APR_EXPORT(int) ap_bucket_getlen(ap_bucket *b)
>  {
> -    if (b) {
> +    if (b && b->getlen) {

Same here.

>          return b->getlen(b);
>      }
>      return 0;
> -}    
> +}
> +
> +APR_EXPORT(ap_status_t) ap_bucket_split(ap_bucket *b, ap_size_t nbyte)
> +{
> +    if (b && b->split) {
> +        return b->split(b, nbyte);
> +    }
> +
> +    /* attempted to split a bucket of a
> +     * color that cannot be split
> +     */

ALL buckets can be split.  I dislike this function.

> +    return APR_BADARG;  /* what is the correct return value here? */

> +    /* attempted to insert into a bucket of a
> +     * color that does not support insertion
> +     */
> +    *w = -1;            /* no data was written */
> +    return APR_BADARG;  /* what is the correct return value here? */

All buckets support insert.  Again, this function isn't necessary.

The change I am considering for the function pointers, is to use a macro
to get at the function.  The reason for this, is that I am considering
some new technology from FreeBSD.  Basically, this would allow us to add
and replace function pointers in the bucket types, without hurting
backwards compatability.  This technology is called kobj or newbus
(depending on where you look), and adds about 10ns to each function call.  

I will keep the list posted as I get more time to look at this stuff.


Ryan Bloom               
406 29th St.
San Francisco, CA 94131

View raw message