[OPEN-ILS-DEV] PATCH: string_array.c

[Bill Erickson]

On 4/28/07, Scott McKellar <mck9 at swbell.net> wrote:
>
> The attached patches tidy up a variety of things in string_array.c
> and the associated header.  In at least some small ways the new
> version will behave differently.  I believe that these changes are
> benign, but you shouldn't take my word for it.
>
> The third attachment is the test driver that I used to verify
> that the modified functions work as intended.
>
>
> 1. I added the const qualifier to a number of function parameters.
> That's why I'm patching the header as well as the implementation.
>
> 2. I added summary comments for some of the functions.
>
> 3. init_string_array() takes an int parameter specifying the initial
> number of strings to be stored.  If this parameter is zero or
> negative, the code won't behave very gracefully.  I added a couple
> of lines to apply a plausible default in this case, so that the
> function will work usefully even in the face of nonsensical input.
> I arbitrarily chose 4 for the default value.  Some other default
> might be better, depending on how string arrays are typically used.
>
> 4. I tightened up various sanity checks, mostly to verify that the
> array member is not NULL.
>
> 5. I tried to eliminate the presumption that the pointer array had
> been initialized to NULLs by setting all bits to zero (since not
> all implementations represent NULL as all-bits-zero).  I considered
> initializing each pointer explicitly to NULL, but that seemed like
> overkill.  It is probably enough to set the first unused pointer
> to NULL.  That's what I did.
>
> 6. In a couple of spots I eliminated the use of the
> osrfStringArrayGetString function, in favor of accessing the array
> directly by subscript.  So far as I can tell, the only reason to use
> this function is to apply sanity checks, especially a range check on
> the subscript.  However in the contexts where this function was
> being used, it could be shown that the sanity checks were unneeded
> because everything was sane, including the subscript.
>
> 7. In osrfStringArrayRemove(), I eliminated the access of the [n+1]
> member (as I discussed in a previous post) by setting array[n]
> explicitly to NULL.  This change avoids the assumption that NULL is
> all-bits-zero.  It also avoids the implicit and non-obvious reliance
> on the fact that string_array_add() never lets the buffer become
> completely full.  The new version of osrfStringArrayRemove() will
> work correctly even if the array is full.
>
> 8. Also in osrfStringArrayRemove(): I changed the behavior of the
> code when the target string is not present in the array.  In such
> a case, the old code unconditionally decrements the size member,
> effectively deleting the last string, though without deallocating
> it.  If the array is empty, the size member becomes negative --
> probably not a good thing.  I doubt that this behavior was intended.
> The new version decrements the size member only when the target
> string is found within the array.
>
> 9. In init_string_array() I considered initializing lhe
> total_string_size member explicitly to zero, rather than rely on
> the implicit initialization by memset().  However I decided to
> leave it alone when I noticed that nothing in string_array.c makes
> any reference to this member.  In fact I grepped the whole codebase
> for this member and got no hits outside of the header file.  We
> can worry about initializing this member if and when it is ever
> used for something.
>
> Scott McKellar
> http://home.swbell.net/mck9/aargh/
>
> Developer's Certificate of Origin 1.1 By making a contribution to this
> project, I certify that:
>
> (a) The contribution was created in whole or in part by me and I have
> the right to submit it under the open source license indicated in the
> file; or
>
> (b) The contribution is based upon previous work that, to the best of
> my knowledge, is covered under an appropriate open source license and I
> have the right under that license to submit that work with
> modifications, whether created in whole or in part by me, under the
> same open source license (unless I am permitted to submit under a
> different license), as indicated in the file; or
>
> (c) The contribution was provided directly to me by some other person
> who certified (a), (b) or (c) and I have not modified it; and
>
> (d) In the case of each of (a), (b), or (c), I understand and agree
> that this project and the contribution are public and that a record of
> the contribution (including all personal information I submit with it,
> including my sign-off) is maintained indefinitely and may be
> redistributed consistent with this project or the open source license
> indicated in the file.
>


As I was going through this patch, I started thinking about how this
string-array code represents a duplication of effort with respect to list
management.  After string-array had been around for a while, I developed a
generic array-based list implementation that works well and probably should
be used as the basis for the string-array API.  I'll look at the feasability
of gutting the string-array code and making those functions list wrappers.

The list code is at OpenSRF/src/libstack/osrf_list*, if anyone cares to eye
that.  It's used quite a bit throughout.

Scott, thanks for keeping us on our toes...

-bill


-- 
Bill Erickson
PINES Systems Developer
Georgia Public Library Service
billserickson at gmail.com
http://open-ils.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://list.georgialibraries.org/pipermail/open-ils-dev/attachments/20070507/afa3b93c/attachment.html