bucket deletion, macro caps

[Cliff Woolley <cl...@yahoo.com>]

I'm still trying to come up with ways to make the bucket API easier to use.
Here are my latest musings:

These sequences are very common:

      APR_BUCKET_REMOVE(e);
      apr_bucket_destroy(e);

The two almost but not quite always go together.  That brings up two issues
in my mind:

(1) It'd be cool to have a third macro that combines the two, both to save
typing and to try to avoid removing without destroying (a memory leak) or
vice versa (a broken brigade).  The only problem I'm having is coming up
with a name for a new macro to combine the two.  I had to resort to the
thesaurus.  =-)  APR_BUCKET_GET_RID_OF, APR_BUCKET_ERADICATE, and
APR_BUCKET_AMPUTATE were probably the most amusing possibilities...
APR_BUCKET_DELETE is probably the most appropriate.  Does that sound good?

(2) I'm ambivalent about the lingering non-capitalization of
apr_bucket_destroy|read|split|setaside|copy.  On the one hand, these things
really are just calls to a single function (well, almost), so lower case is
okay.  And it's consistent with apr_brigade_destroy, apr_bucket_foo_create,
and so on, which really ARE functions.  On the other hand, they're almost
the only macros with lowercase names.  I guess the only reason they're lower
case is that I left them that way when I converted them to macros in the
first place--it was easier at the time because it didn't affect any of the
callers.  But now I'm really torn.  It seems like it'd be a real pain for
someone new to the bucket API to remember which case goes with which
function/macro name.  I can remember, and I'm sure most of you can too, but
then again we've been looking at these things since they were created.  What
do you all think?  Is the documentation sufficient?  Or is a mass rename in
order, on the premise that consistency is a Good Thing?  Which way IS
consistent?  I could be convinced either way.

--Cliff

---------------------------------------------------
    Cliff Woolley
    cliffwoolley@yahoo.com
    804-244-8615
    Charlottesville, VA

Re: bucket deletion, macro caps

["Roy T. Fielding" <fi...@ebuilt.com>]

In general, any macro function that might eventually be replaced by
a real function should have a name in the same case as a real function.
All caps should only be used when you want the API user to know that
it is a defined symbol/macro.

....Roy

RE: bucket deletion, macro caps

[Cliff Woolley <cl...@yahoo.com>]

> -----Original Message-----
> Cliff, you just hit on two things that I have spent a lot of time thinking
> about, so I'll give my opinions to add to your own.

Glad it's not just me.  =-)

> I realized that combining them wasn't as easy as you would
> think.  The first option is to always destroy whenever you call
> APR_BUCKET_REMOVE.  This doesn't work though, because it means that we
> can't move a bucket from one brigade to another
> [APR_BUCKET_REMOVE(e);APR_BRIGADE_ADD(...)].

Clearly.  And there are probably some other valid reasons to want to do
this...  Anyway, you're right, this wouldn't work.

> The second option is much
> more appealing.  I would just put a call to APR_BUCKET_REMOVE(e) inside of
> apr_bucket_destroy().  This is safe to do, because we can't destroy a
> bucket without removing it from the brigade, and if the bucket isn't in
> the brigade, the macro is basically a no-op.

I thought about that, too.  But I was worried that it might be possible to
create a bucket and then immediately destroy it, and the ->prev and ->next
pointers might be garbage.  Upon further inspection, I now see that
apr_bucket_do_create uses APR_RING_ELEM_INIT, so this would work.

So it's either overloading apr_bucket_destroy() or adding a third macro.
I'm game either way.

> > (2) I'm ambivalent about the lingering non-capitalization of
> > apr_bucket_destroy|read|split|setaside|copy.  On the one hand,
>
> I am against renaming personally, but if you are going to rename, do it
> now.  :-)

Somehow I knew you'd say that.  =-)  I'm starting to think that if we take
care of (1), then (2) won't matter all that much, and we can get by with
ignoring it.  (Besides, after more thought, I'm swinging back toward
thinking that it's more important for apr_bucket_destroy() to match
apr_bucket_foo_create() than for it to be capitalized just because it's a
macro.)

--Cliff

Re: bucket deletion, macro caps

[rb...@covalent.net]

Cliff, you just hit on two things that I have spent a lot of time thinking
about, so I'll give my opinions to add to your own.

> I'm still trying to come up with ways to make the bucket API easier to use.
> Here are my latest musings:
> 
> These sequences are very common:
> 
>       APR_BUCKET_REMOVE(e);
>       apr_bucket_destroy(e);
> 
> The two almost but not quite always go together.  That brings up two issues
> in my mind:
> 
> (1) It'd be cool to have a third macro that combines the two, both to save
> typing and to try to avoid removing without destroying (a memory leak) or
> vice versa (a broken brigade).  The only problem I'm having is coming up
> with a name for a new macro to combine the two.  I had to resort to the
> thesaurus.  =-)  APR_BUCKET_GET_RID_OF, APR_BUCKET_ERADICATE, and
> APR_BUCKET_AMPUTATE were probably the most amusing possibilities...
> APR_BUCKET_DELETE is probably the most appropriate.  Does that sound good?

A few weeks/months ago, I was going to combine these, but never got to
it.  Then I realized that combining them wasn't as easy as you would
think.  The first option is to always destroy whenever you call
APR_BUCKET_REMOVE.  This doesn't work though, because it means that we
can't move a bucket from one brigade to another
[APR_BUCKET_REMOVE(e);APR_BRIGADE_ADD(...)].  The second option is much
more appealing.  I would just put a call to APR_BUCKET_REMOVE(e) inside of
apr_bucket_destroy().  This is safe to do, because we can't destroy a
bucket without removing it from the brigade, and if the bucket isn't in
the brigade, the macro is basically a no-op.

> (2) I'm ambivalent about the lingering non-capitalization of
> apr_bucket_destroy|read|split|setaside|copy.  On the one hand, these things
> really are just calls to a single function (well, almost), so lower case is
> okay.  And it's consistent with apr_brigade_destroy, apr_bucket_foo_create,
> and so on, which really ARE functions.  On the other hand, they're almost
> the only macros with lowercase names.  I guess the only reason they're lower
> case is that I left them that way when I converted them to macros in the
> first place--it was easier at the time because it didn't affect any of the
> callers.  But now I'm really torn.  It seems like it'd be a real pain for
> someone new to the bucket API to remember which case goes with which
> function/macro name.  I can remember, and I'm sure most of you can too, but
> then again we've been looking at these things since they were created.  What
> do you all think?  Is the documentation sufficient?  Or is a mass rename in
> order, on the premise that consistency is a Good Thing?  Which way IS
> consistent?  I could be convinced either way.

I am against renaming personally, but if you are going to rename, do it
now.  :-)

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------