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

[Bill Erickson]

On 5/7/07, Bill Erickson <billserickson at gmail.com> wrote:
>
>
>
> 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



I applied some of the additional sanity checks from these patches to
osrf_list* and made more judicious use of the const qualifier.  I have a
version of string_array that acts as a wrapper to osrf_list, but given the
awkward state of the libopensrf build, I don't plan in importing it until
we've consolidated the build into a single directory (as discussed in a
previous email to this list).

-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/9e9afa6a/attachment-0001.html