xstrcat function

[=?ISO-8859-1?Q?Leonardo_Javier_Bel=E9n?=]

I've been studing the way to have a more useful strcat function and I
realized that I need more than one argument to handle at a time, and
also a way to resize the pointer automatically so I can forget the
memory handling problem.

So I constructed a new xstrcat function but, although works pretty
well in most environments, I discover that it works erratically when I
try to add the result var on the list of strings to add (i mean if I
do a xstrcat(&tmp, tmp, NULL)), so I send this to the community to see
if someone with more experience than me can help.
Cheers,
Leo

char *
xstrcat (char **dest, char *str, ...)
{
va_list va;
size_t length = 0;
char *ptr, *tmp;

if (!str)
return (char *) NULL;

length += strlen (str);

va_start (va, str);
while ((tmp = va_arg (va, char *)))
length += strlen (tmp);
va_end (va);

if (!*dest)
*dest = malloc (length + 1);
else
*dest = realloc (*dest, (length + 1));

if (!dest)
return (char *) NULL;

ptr = *dest;

for (tmp = str; *tmp; tmp++)
*ptr++ = *tmp;

va_start (va, str);
while ((tmp = va_arg (va, char *)))
{
while (*tmp)
*ptr++ = *tmp++;
}
va_end (va);
*ptr = '\0';
return *dest;
}

 

[Ben Pfaff]

So I constructed a new xstrcat function but, although works pretty
well in most environments, I discover that it works erratically when I
try to add the result var on the list of strings to add (i mean if I
do a xstrcat(&tmp, tmp, NULL)), so I send this to the community to see
if someone with more experience than me can help.

char *
xstrcat (char **dest, char *str, ...)
{
va_list va;
size_t length = 0;
char *ptr, *tmp;

if (!str)
return (char *) NULL;

The cast is gratuitous.

length += strlen (str);

va_start (va, str);
while ((tmp = va_arg (va, char *)))
length += strlen (tmp);
va_end (va);

if (!*dest)
*dest = malloc (length + 1);
else
*dest = realloc (*dest, (length + 1));

1. realloc() acts like malloc() when the first argument is a null
pointer, so there's no need to switch to malloc() for that
case.

2. If realloc() fails, you probably just leaked memory, because
you overwrote the original pointer.

if (!dest)

This does not test what you think it tests. If allocation
failed, *dest will be a null pointer, not dest.

return (char *) NULL;

Gratuitous cast.

ptr = *dest;

for (tmp = str; *tmp; tmp++)
*ptr++ = *tmp;
va_start (va, str);
while ((tmp = va_arg (va, char *)))
{
while (*tmp)
*ptr++ = *tmp++;
}
va_end (va);
*ptr = '\0';

The rest of this looks okay, though I'd prefer strcpy() followed
by strchr() over explicit loops.