D
Dave Thompson
Thanks to Michael Mair, Barry Schwartz, and Spiros Bousbouras for their
suggestions.
OT: It appears that Google's interface has indeed been fixed, and it's
actually very impressive now.
Here's my new version:
#include <stdlib.h>
#include <string.h>
#include "dynamic_buffer.h"
struct dbuffer_s {
size_t size;
size_t max;
size_t num;
void *content;
This is completely different from your earlier version. It is a
pointer to space allocated somewhere else, not adjacent.
};
void *create_buffer (size_t size, size_t max)
{
struct dbuffer_s *tmp = malloc (sizeof *tmp);
if (tmp != NULL)
{
tmp->size = size; /* Size of each item */
tmp->max = max; /* Maximum # of items */
tmp->num = 0; /* Actual # of items */
tmp->content = calloc (size * max);
That's wrong. Either malloc (size * max) or calloc (size, max) with
two separate arguments. Since your logic tries to ensure that each new
'item's worth of space is written before it can be read, the extra
work calloc (probably) does is wasted and malloc is better.
Oh, and either way should probably check for overflow:
if( max > SIZE_MAX / size ){ give up now }
In nonerror case, this returns the value of the pointer tmp->content,if (tmp->content == NULL) {
free (tmp);
tmp = NULL;
}
}
return (tmp != NULL) ? tmp->content : NULL;
}
which is the address of space somewhere completely unrelated to the
address of the space for struct dbuffer_s that tmp points to.
/* This function will return three possible things:
* The new maximum on success
* The old maximum on failure (or itm == NULL)
How is the caller supposed to distinguish these? Is it required to
keep track of the current usage? If so, doesn't that reduce the
benefit you intended to provide by encapsulating this functionality?
* 0 on buf == NULL
*/
size_t append_item (void **buf, void *itm)
{
static struct dbuffer_s stmp; /* Used for length calculations. */
size_t distance_back = (size_t) &stmp.content - (size_t) &stmp;
Personally I would make this an assert() or some similar butstruct dbuffer_s *tmp;
/* Initial error conditions */
if (buf == NULL)
return 0;
project/environment-specific fatal error. If the caller is so screwed
up as to be trying to use an unallocated buffer, it is probably so
screwed up it won't handle an error return usefully.
Assuming tmp is the value returned from create_buffer, this won't worktmp = (struct dbuffer_s *) ((unsigned char *) buf - distance_back);
at all. If you want to do this kind of kludge, and accept the issues
about ensuring alignment which <ObStd> can't be done for all cases
guaranteed portably but </> can be done for most common systems, you
need to allocate _one_ chunk of space big enough for the header plus
padding plus the data, give the caller the address to the data part,
and step backward from that to find the header -- leaving any excess
padding, unusually, _before_ the header.
And in that case you don't need to do it explicitly; assuming buf
actually points to the caller's void* pointer (NOT portably a data*
pointer, see my previous post) just use:
struct dbuffer_s * hdrp = (struct dbuffer_s *) *buf - 1;
Or you can (and when I did this I preferred to):
struct dbuffer_s * hdrp = /* (struct dbuffer_s *) */ *buf;
and then use hdrp[-1].field rather than hdrp->field .
if (itm == NULL)
return tmp->size;
/* Here's where the (attempted) reallocation starts: */
if (tmp->num == tmp->max)
{
size_t max = tmp->max * 2;
Should check for overflow: old tmp->max > SIZE_MAX/2 on the count, but
old tmp->max > SIZE_MAX/size/2 for the resulting allocation. Though on
most systems you will run out of available memory and realloc() will
fail sufficiently before you run out of bits in size_t.
This initialization is useless; you immediately overwrite it.void *ptmp = tmp->content; /* tmp->content is *buf. */
There's no point (re)trying the old tmp->max; see my previous post.ptmp = realloc (tmp->content, max * tmp->size);
while (ptmp == NULL)
{
ptmp = realloc (tmp->content, --max * tmp->size);
/* If we failed, we failed. */
if (max == tmp->max)
break;
}
And update tmp->max, see my previous post./* If we pulled anything off, reflect it in tmp: */
if (max > tmp->max)
tmp->content = ptmp;
}
That's backwards; the first argument is the destination, which should/* Now the actual addition code: */
if (tmp->num < tmp->max)
memmove (itm, (unsigned char *) tmp->content + tmp->num++,
tmp->size);
be (anychar*)content + (num++) * size (note scaling by size), and the
second argument should be itm, the object you are adding.
And as I previously posted, these should never overlap for a valid
caller, so you only need memcpy() not memmove(), but will screw up if
the caller gives you an element in the old pre-reallocated array.
return tmp->size;
}
- David.Thompson1 at worldnet.att.net