HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

A variadic C function for concatenating multiple strings

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
concatenatingfunctionvariadicformultiplestrings

Problem

I have this function that takes a number \$N\$, and \$N\$ C strings, concatenates and returns the result:

#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:

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.