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

[Scott McKellar]

--- "David J. Fiander" <djfiander at fastmail.fm> wrote:

> Well, BUFFER_MAX_SIZE is not necessarily an integer literal; it might
> (and 
> probably does) contain a cast, probably to size_t.

BUFFER_MAX_SIZE is currently defined in utils.h as a bare integer 
literal, value 10485760.  Of course that could change.

The problem with the current code is that it *assumes* that this
macro evaluates to an int, or at least to something that looks a lot
like an int.  On many UNIX or Linux systems it won't really matter
whether it's an int or a long or a long long, nor will it matter
(in this case) whether it's signed or unsigned -- it will all look
the same in the compiled code.

However as you point out some systems have different sizes for these
things.

The code should not assume anything about the type.  It should use a
cast to coerce the type to something that matches the conversion 
specification.

<snip>
 
> >     You would likely want to wrap realloc() in a safe_realloc
> function, or
> >     an OSRF_REALLOC macro.
> 
> The only reason that realloc() might fail is because there's not
> enough 
> memory for the new buffer size.  I suspect that if we get into that 
> situation, the simplest thing to do is to die:
> 
> 	if ((p = realloc(p, newsize)) == NULL) {
> 		die("ack!");
> 	}

Agreed.  In any case the policy should be the same as for a 
failure of malloc(), which in safe_malloc() is to issue a 
message and call exit(99).

Incidentally in my earlier note I didn't express myself
very well.  I said that you could avoid calling strcpy() 
and free() when realloc() was able to expand the buffer
in place.  Actually you could avoid both of these functions
in either case, at least in your own code.  realloc() will
internally call or not call them (or the equivalents) as
needed.

<snip>
 
> Does the count gb->n_used include the '\0' at the end of the string,
> or 
> not?  A zero-length string stored in a buffer is using one byte of
> the 
> buffer, but to concatenate a string onto the zero-length string we
> need to 
> adjust the source pointer by 0, not by 1 (induction is wonderful). 

As I recall, n_used does not include the terminal nul.  I'll have to
check again when I have a chance.  If it does include the terminal
nul then my suggestion can be adjusted accordingly.

> The 
> nice thing about strcat() is that it eliminates the need to worry
> about 
> this sort of fussy detail, even if it does cost a few processor
> cycles.

That's a judgement call.  My own bias is that, within reason, I
don't mind being fussy, especially in a low-level routine that
doesn't receive a lot of ongoing maintenance.  The price of fussiness
need be paid only once, but the price of unfussiness will be paid
over and over.

Thanks for your comments.

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