[OPEN-ILS-DEV] C nits: object.c (Part 1)

[Scott McKellar]

I have some remarks about the __tabs() function in object.c.

First, in pedantic mode, I must point out that the name "__tabs" is
reserved for the implementation, since it begins with two
underscores.

Second: I suspect that this function was not intended to be called
from outside the object.c module (which would probably explain why it
has a funny name).  It is not declared in object.h, which is where I
would expect to see it if it were intended to have external linkage.

If so, then it should be declared static, so that its scope is 
confined to the translation unit in which it appears.

Third, it is needlessly inefficient.  It adds successive pieces of
white space in a loop, instead of calculating how many spaces it needs
from the outset.  It requires at least three calls to malloc(): one
to allocate a growing_buffer, one to allocate the growing_buffer's
internal buffer, and one (via strdup()) to make a copy of that 
internal buffer.  In the unlikely event that you need more than 
seven tabs, the growing_buffer will call malloc() again to expand
the internal buffer.

I propose the following as a drop-in replacement:

#define CHARS_PER_TAB 3

static char * tabs( int count ) {
	size_t blank_count;

	if( count > 0 )
		blank_count = CHARS_PER_TAB * count;
	else
		blank_count = 0;
	
	char * final = OSRF_MALLOC( blank_count + 1 );
	memset( final, ' ', blank_count );
	final[ blank_count ] = '\0';
	return final;
}

Here I calculate how big a buffer I need, allocate it once, fill it
with blank spaces, and add a terminal nul.  I use OSRF_MALLOC, but
safe_malloc() would work too.  In testing I used a plain malloc() so 
that I wouldn't have to bring in a lot of other machinery.

The remaining inefficiency is a useless call to memset() buried in
OSRF_MALLOC.  As long as OSRF_MALLOC and safe_malloc() call memset(),
I can eliminate that overhead only by calling malloc() directly, and
I suspect that you'd rather not do that.

Scott McKellar
http://home.swbell.net/mck9/aargh/