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

[Scott McKellar]

Here are some miscellaneous observations on string_array.c, none of 
them terribly exciting.


1. Most of the osrfStringArray functions merely pass their arguments
to corresponding string_array functions.  They could be defined as
macros in order to eliminate a layer of function call.


2. Several pointer parameters should carry the const qualifier, as in
"const char *" instead of plain "char *":

osrfStringArrayAdd()        second parameter
string_array_add()          second parameter
osrfStringArrayGetString()  first parameter
string_array_get_string()   first parameter
osrfStringArrayContains()   both parameters
osrfStringArrayRemove()     second parameter


3. init_string_array() initializes the size and arr_size members, 
but not the total_string_size member.  It is initialized only 
indirectly to zero when OSRF_MALLOC fills the first allocated chunk 
of memory with binary zeros.  Tastes may differ, but I would prefer
to initialize total_string_size explicitly, rather than rely on an
accidental side effect of a memset() buried in a macro.

(My secret mission, of course, is to eliminate all reliance on that
memset(), so that it can be eliminated entirely.)


4. Every function that receives a pointer to an osrfStringArray or
string_array as a parameter starts out by verifying that the pointer
is not NULL.  It should also verify that arr->array is not null.
There is nothing stopping the calling code from passing a pointer 
to a structure that is internally corrupt.


5. string_array_add() verifies that the string passed to it is not
empty.  It does so by calling strlen(), but all it needs to do is
examine the first character.  (It may be that the compiler does this
optimization anyway; I don't know.)


6. I'm puzzled that osrfStringArrayGetString() and 
string_array_get_string() are declared with external linkage.  They
take as a parameter an index to the internal pointer array.  However
no function tells the calling code what the index is for any given 
string.  The only way the calling code can obtain such an index is by 
mucking about with the internals of the string_array -- a practice 
that you probably don't want to encourage.

I suspect, rather, that these functions were intended only for
internal use within string_array.c.  In that case they should be
declared static, and their declarations should be removed from
string_array.h.


7. Two functions call osrfStringArrayGetString() in order to fetch
a pointer from the array for a given subscript, instead of
referencing the array directly.  This layer of function call adds
a sanity check and some bounds checking.

However this validation is completely unnecessary in these contexts.
We have already verified that arr is not NULL (and we should have
also verified that arr->array is not NULL, as noted above).  Due
to the constraints imposed by the enclosing loop, we also know that
the subscript is in bounds.  The call to osrfStringArrayGetString()
adds overhead to no purpose, unless of course the compiler optimizes
it out of existence.

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