patterncMinor
A variadic C function for concatenating multiple strings
Viewed 0 times
concatenatingfunctionvariadicformultiplestrings
Problem
I have this function that takes a number \$N\$, and \$N\$ C strings, concatenates and returns the result:
As always, please tell me anything that comes to mind.
#include
#include
#include
#include
char* mystrcat(int count, ...)
{
char** p;
char* result;
char* ptr;
size_t* len_array;
va_list ap;
int j;
size_t total_length;
if (count < 1)
{
return "";
}
va_start(ap, count);
p = malloc(sizeof(char*) * count);
len_array = calloc(count, sizeof(size_t));
total_length = 0;
for (j = 0; j != count; ++j)
{
p[j] = va_arg(ap, char*);
total_length += (len_array[j] = strlen(p[j]));
}
result = malloc(sizeof(char) * (total_length + 1));
ptr = result;
for (j = 0; j != count; ++j)
{
strcpy(ptr, p[j]);
ptr += len_array[j];
}
*ptr = '\0';
va_end(ap);
return result;
}
int main(int argc, char* argv[]) {
puts(mystrcat(5, "Hello", ", ", "world", ", ", "friends!"));
}As always, please tell me anything that comes to mind.
Solution
Always return allocated string or never do it
This code here:
is bad because you return a static string whereas the normal case returns a string allocated by
or by just returning NULL:
Memory leaks
Your function never frees
Rewrite
After fixing the above issues, my rewrite of your function would look like this:
This code here:
if (count < 1)
{
return "";
}is bad because you return a static string whereas the normal case returns a string allocated by
malloc. If the caller then tries to free this static string, it will cause some kind of error. You can fix this by either returning an allocated empty string:if (count < 1)
{
return calloc(1, 1);
}or by just returning NULL:
if (count < 1)
{
return NULL;
}Memory leaks
Your function never frees
p or len_array. Actually, I would probably not even use temporary arrays like these. If I were to write this function, I would measure the total length on one pass, and then copy the strings on a second pass.Rewrite
After fixing the above issues, my rewrite of your function would look like this:
char *mystrcat(int count, ...)
{
va_list ap;
size_t len = 0;
if (count < 1)
return NULL;
// First, measure the total length required.
va_start(ap, count);
for (int i=0; i < count; i++) {
const char *s = va_arg(ap, char *);
len += strlen(s);
}
va_end(ap);
// Allocate return buffer.
char *ret = malloc(len + 1);
if (ret == NULL)
return NULL;
// Concatenate all the strings into the return buffer.
char *dst = ret;
va_start(ap, count);
for (int i=0; i < count; i++) {
const char *src = va_arg(ap, char *);
// This loop is a strcpy.
while (*dst++ = *src++);
dst--;
}
va_end(ap);
return ret;
}Code Snippets
if (count < 1)
{
return "";
}if (count < 1)
{
return calloc(1, 1);
}if (count < 1)
{
return NULL;
}char *mystrcat(int count, ...)
{
va_list ap;
size_t len = 0;
if (count < 1)
return NULL;
// First, measure the total length required.
va_start(ap, count);
for (int i=0; i < count; i++) {
const char *s = va_arg(ap, char *);
len += strlen(s);
}
va_end(ap);
// Allocate return buffer.
char *ret = malloc(len + 1);
if (ret == NULL)
return NULL;
// Concatenate all the strings into the return buffer.
char *dst = ret;
va_start(ap, count);
for (int i=0; i < count; i++) {
const char *src = va_arg(ap, char *);
// This loop is a strcpy.
while (*dst++ = *src++);
dst--;
}
va_end(ap);
return ret;
}Context
StackExchange Code Review Q#142638, answer score: 6
Revisions (0)
No revisions yet.