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

[Bill Erickson]

Scott,

This is great, thanks.  The growing_buffer is used widely, as you say, so
starting there is a good place.  Also, a little premature optimization won't
hurt with such frequently accessed code.  I won't bother going through each
point individually,  because I think it's all generally good advice.

A lot of the code you're looking at is mine, and you can probably tell that
I was not "raised" as a C programmer, though I'm quite fond of the language,
so your pointers on style in addition to optimization are greatly
appreciated.

I'll be returning to "C nits: utils.c (Part 1)" as time permits.

Thanks again,

-bill
**

On 3/22/07, Scott McKellar <mck9 at swbell.net> wrote:
>
> 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/
>
>


-- 
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/20070323/340ecfc1/attachment-0001.html