[OPEN-ILS-DEV] C nits: utils.c (Part 2)

[Scott McKellar]

Gentlemen:

I have turned my attention to the growing_buffer routines.  I believe
I see some opportunities for improvement, starting with buffer_add().

Some of my suggestions may fall into the category of premature
optimization.  However I suspect that the growing_buffer routines are 
so widely used that even a small performance tweak might add up to
something significant.

First: the second parameter absolutely should be a pointer to const
char.  Without the const qualifier in the signature, the compiler will
not let you pass a const pointer, and there's no reason to restrict
yourself that way.

A pedantic point: data_len and total_len should be declared as size_t
rather than int.  That's what strlen() returns, and neither will ever 
have a negative value, so they might as well be unsigned.

For that matter the size and n_used members of growing_buffer should
probably be size_t as well, and buffer_add() should return a size_t.
These are the sorts of things for which size_t was intended.

Granted, it's unlikely that leaving them as ints will ever do any 
harm.  It's a stylistic point upon which reasonable people may
disagree.

A second pedantic point: In your error message you format
BUFFER_MAX_SIZE as an int, with the "%d" format.  I'm not sure what
type the compiler applies to an integer literal, such as 
BUFFER_MAX_SIZE expands to.  Probably it's an int unless it's too
big to be an int.

In any case there are systems where the value of BUFFER_MAX_SIZE won't
fit into an int.  Probably none of them would ever run Open-ILS.
However I can imagine someone trying to port fragments of your 
code to MS-DOS, for example.  Of course, he or she would probably 
have to change the value of BUFFER_MAX_SIZE anyway.

In any case, the pedantically correct thing to do is to use the "%ld" 
format, and cast BUFFER_MAX_SIZE to a long.  Alternatively you could 
append an "L" to the definition of this macro.  You could also use
the "%lu" format and turn BUFFER_MAX_SIZE to an unsigned long.

When you issue this error message, you also destroy the growing_buffer
and its contents.  I don't understand why you do that.  Why throw
away something you already have?  I suppose the idea is to enforce
discipline by making a coredump likely if the calling code doesn't
scrupulously check the return value.  However I suspect that this
error happens rarely enough that the enforcement is not very 
effective.

Now for the good stuff.

When you expand the buffer, consider using realloc() instead of 
malloc().  If realloc() can expand the buffer in place instead of
allocating a completely new one, then you can avoid doing the 
strcpy() and the free().  You may also reduce the fragmentation of
memory.

Whether this measure helps will depend on how often realloc() can 
expand the buffer in place.  I have no way to guess how often that 
happens.  It probably depends on the application.

You would likely want to wrap realloc() in a safe_realloc function, or
an OSRF_REALLOC macro.

Finally: when you append the string to the buffer, use strcpy() 
instead of strcat():

    strcpy( gb->buf + gb->n_used, data );

Rationale: strcat() has to spend CPU cycles finding the end of the
original string.  But you already know where the end of the string
is.  You might as well put that information to good use.

Scott McKellar
mck9 at swbell.net/mck9/aargh/