[OPEN-ILS-DEV] PATCH: utils.c (faster buffer_add_char)

[Mike Rylander]

On Dec 9, 2007 7:04 PM, Scott McKellar <mck9 at swbell.net> wrote:
> This patch provides a faster implementation of buffer_add_char().

Applied.
[snip]

> I have three further suggestions for the handling of growing_buffers.
> I didn't want to make these changes on my own without discussion.
>
> 1. buffer_add_char() should return void instead of int.  So far as I
> can tell, no code looks at the return value anyway -- nor would it
> make sense to, since it always returns the same value.

Well, if we were being Good(tm) we would check this, though once you
run into an out-of-memory state you're pretty much dead in the water.
However, because of the inconsistency it would be difficult to apply
checks anyway.  Because of this, I've made both buffer_add and the new
buffer_add_char, as well as buffer_fadd (by way of buffer_add) and
buffer_reset, return -1 on buffer expansion (or buffer reset) error
and the total length of the string upon success.  This also rectifies
your point 4 below, I think.

>
> 2. It's also doubtful whether we ever check the return value of
> buffer_add(), buffer_fadd(), or buffer_reset(), though I haven't
> looked.  We could simplify the code a bit by letting them return void.
>

We could, but IMO it's better to have an unused sanity check (and work
towards improving the calling code) than to remove it.  But, yeah, I
couldn't find a single assignment from buffer_add and friends for the
purpose of checking the return value.

> 3. If the amount of buffer space needed is greater than
> BUFFER_MAX_SIZE, we not only issue an error message, but also free
> the original buffer.  I don't see any reason to do so, unless we
> want to make it nearly impossible for the client code to continue.
> We could get approximately the same result more gracefully by calling
> exit() or abort().  Alternatively we could leave the original buffer
> unchanged, and rely on the client code to check the return value
> (which it seldom or never does in practice).
>

I could get behind leaving the buffer unchanged and requiring the
calling code to check for the condition and do something useful.

> 4. If we ask buffer_add to append an empty string, it returns 0,
> but if we ask it to append a non-empty string, it returns the length
> of the newly augmented string.  If we never look at the return value
> then it doesn't matter what we return.  However it would be more
> consistent to return the original string length if the string to be
> appended is empty.  Appending an empty string is not an error, it's
> just a special case.
>

Agreed.  Returning -1 from these fixes the issue, and since the return
value isn't used it won't break any existing code.

Thanks Scott.

-- 
Mike Rylander
 | VP, Research and Design
 | Equinox Software, Inc. / The Evergreen Experts
 | phone:  1-877-OPEN-ILS (673-6457)
 | email:  miker at esilibrary.com
 | web:  http://www.esilibrary.com