UPDATED: str_join() function, not present in standard C library

Question

This function str_join() is an updated version of my earlier function str_join() [Found here: https://codereview.stackexchange.com/questions/270759/str-join-function-not-present-in-standard-c-library].

This version has all bugs fixed and has incorporated some review comments.

Syntax: char *str_join(unsigned int skip_null_and_empty_input_strings, const char *delim, long num_args, ...);

The description of the function str_join() is in the header file - "str_join.h".

The code is below:

---

str_join.c

#include "str_join.h"

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>

/* The description of the function str_join() is in the header file - "str_join.h" */
char *str_join(unsigned int skip_null_and_empty_input_strings, const char *delim, long num_args, ...)
{

    va_list valist;
    long i = 0;
    size_t iica = 0; // iica - index into character array
    size_t len = 0;
    size_t delim_len = 0;
    size_t total_len = 0;
    long num_delim_to_concat = -1;
    char *new_char_array = NULL;
    char *temp = NULL;

    if (num_args <= 0)
        return NULL;

    if (delim) {
        delim_len = strlen(delim);
    }

    va_start(valist, num_args);
    for (i = 0; i < num_args; i++) {

        temp = va_arg(valist, char *);

        if (skip_null_and_empty_input_strings) {
            if ((!temp) || (!*temp))
                continue;
        }

        if ((!temp) || (!*temp))
            len = 0;
        else 
            len = strlen(temp);

        total_len = total_len + len;
        num_delim_to_concat = num_delim_to_concat + 1;
        if (num_delim_to_concat > 0)
            total_len = total_len + delim_len;

    }
    va_end(valist);

    if (total_len == 0)
        return NULL;

    total_len = total_len + 1; // 1 extra for terminating null byte

    new_char_array = malloc(total_len);
    if (!new_char_array)
        return NULL;

    va_start(valist, num_args);
    for (i = 0; i < num_args; i++) {

        temp = va_arg(valist, char *);

        if (skip_null_and_empty_input_strings) {
            if ((!temp) || (!*temp))
                continue;
        }

        if ((!temp) || (!*temp))
            len = 0;
        else 
            len = strlen(temp);

        memmove(&(new_char_array[iica]), temp, len);
        iica = iica + len;

        if (num_delim_to_concat > 0) {
            memmove(&(new_char_array[iica]), delim, delim_len);
            iica = iica + delim_len;
            num_delim_to_concat = num_delim_to_concat - 1;
        }

    }
    va_end(valist);

    new_char_array[iica] = 0;

    return new_char_array;

} // end of str_join

---

str_join.h

#ifndef STR_JOIN_H
#define STR_JOIN_H

/*
 * char *str_join(unsigned int skip_null_and_empty_input_strings, const char *delim, long num_args, ...):
 *
 * Parameters:
 *
 *      num_args: number of variable arguments that are passed to this function
 *                excluding the 'delim' string.
 *      ...: Variable number of "char *" pointers.
 *
 * Description:
 *
 *      Function str_join() concatenates all the strings/character arrays passed
 *      to it. If 'delim' is not NULL or not empty then between every two strings,
 *      the 'delim' string is concatenated.
 *
 *      If skip_null_and_empty_input_strings is zero then this means that NULL/empty
 *      strings should be considered valid strings for the purpose of concatenating
 *      'delim' string - this means that if there is a NON-NULL/non-empty string
 *      in the variable arguments list which is then followed or preceded by a
 *      NULL/empty string then one 'delim' string will be concatenated between
 *      NON-NULL/non-empty string and NULL/empty string. This can be useful in
 *      case columns of a database are concatenated to form a record which will
 *      then be written in a file - so here, a column containing NULL/empty value
 *      will be represented as empty by having two consecutive 'delim' strings.
 *
 *      If skip_null_and_empty_input_strings is non-zero then this means that
 *      NULL/empty strings should be skipped and no 'delim' string should be
 *      concatenated for them.
 *
 *      Function str_join() allocates a new character array whose size is equal
 *      to the sum of the lengths of all strings passed to it plus 1 (extra 1
 *      for terminating null byte). It then concatenates all the strings passed
 *      to it (these strings are separated by 'delim' string but please see above
 *      for NULL/empty strings) into the newly allocated character array and then
 *      returns the pointer to the newly allocated character array. If memory
 *      allocation fails then NULL is returned.
 *
 *      It is the responsibility of the caller to free the allocated memory (that
 *      is, to free the returned pointer from this function).
 */
char *str_join(unsigned int skip_null_and_empty_input_strings, const char *delim, long num_args, ...);

#endif

---

test_str_join.c

#include "str_join.h"

#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int mystrcmp(const char *result, const char *expected);
void verify_result(char *result, const char *expected, int line_num, const char *file_name);

int mystrcmp(const char *result, const char *expected)
{
    if ((!result) && (!expected))
        return 0;
    else if ((!result) || (!expected))
        return 1;
    else
        return strcmp(result, expected);
} // end of mystrcmp

void verify_result(char *result, const char *expected, int line_num, const char *file_name)
{

    int ret_val = -1;

    printf("\n");
    if (result) printf("Result = \"%s\"\n", result); else printf("Result = NULL\n");
    if (expected) printf("Expected = \"%s\"\n", expected); else printf("Expected = NULL\n");
    ret_val = mystrcmp(result, expected);
    free(result);
    if (ret_val != 0) {
        printf("Test case failed. Check the test case just before line number %d in file \"%s\".\n", line_num, file_name);
        printf("\nExiting ..\n\n");
        exit(1);
    } else {
        printf("Test case passed.\n");
    }

} // end of verify_result

int main(void)
{

    char *result = NULL;
    char *expected = NULL;

    result = str_join(0, "-", 0);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 0, "abc");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 0, "");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 0, NULL);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 1, "abc");
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 1, "");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 1, NULL);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 2, "abc", "123");
    expected = "abc-123";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 2, "abc", "");
    expected = "abc-";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 2, NULL, "abc");
    expected = "-abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 2, "", NULL);
    expected = "-";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "abc", "123", "fgh");
    expected = "abc-123-fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "", "123", "fgh");
    expected = "-123-fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "abc", NULL, "fgh");
    expected = "abc--fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "abc", "123", "");
    expected = "abc-123-";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, NULL, "NULL", "");
    expected = "-NULL-";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "abc", "", NULL);
    expected = "abc--";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-", 3, "", NULL, "abc");
    expected = "--abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-?-", 4, "a", "b", "cd", "e");
    expected = "a-?-b-?-cd-?-e";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-?-", 4, "a", NULL, "cd", "e");
    expected = "a-?--?-cd-?-e";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-?-", 4, "a", "", "cd", "");
    expected = "a-?--?-cd-?-";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "-?-", 10, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10");
    expected = "1-?-2-?-3-?-4-?-5-?-6-?-7-?-8-?-9-?-10";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "", 4, "a", "b", "cd", "e");
    expected = "abcde";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, NULL, 4, "a", NULL, "cd", "e");
    expected = "acde";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, NULL, 4, "a", "", "cd", "");
    expected = "acd";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(0, "", 10, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10");
    expected = "12345678910";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 0);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 0, "abc");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 0, "");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 0, NULL);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 1, "abc");
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 1, "");
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 1, NULL);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 2, "abc", "123");
    expected = "abc-123";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 2, "abc", "");
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 2, NULL, "abc");
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 2, "", NULL);
    expected = NULL;
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "abc", "123", "fgh");
    expected = "abc-123-fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "", "123", "fgh");
    expected = "123-fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "abc", NULL, "fgh");
    expected = "abc-fgh";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "abc", "123", "");
    expected = "abc-123";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, NULL, "NULL", "");
    expected = "NULL";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "abc", "", NULL);
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-", 3, "", NULL, "abc");
    expected = "abc";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-?-", 4, "a", "b", "cd", "e");
    expected = "a-?-b-?-cd-?-e";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-?-", 4, "a", NULL, "cd", "e");
    expected = "a-?-cd-?-e";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-?-", 4, "a", "", "cd", "");
    expected = "a-?-cd";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "-?-", 10, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10");
    expected = "1-?-2-?-3-?-4-?-5-?-6-?-7-?-8-?-9-?-10";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "", 4, "a", "b", "cd", "e");
    expected = "abcde";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, NULL, 4, "a", NULL, "cd", "e");
    expected = "acde";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, NULL, 4, "a", "", "cd", "");
    expected = "acd";
    verify_result(result, expected, __LINE__, __FILE__);

    result = str_join(1, "", 10, "1", "2", "3", "4", "5", "6", "7", "8", "9", "10");
    expected = "12345678910";
    verify_result(result, expected, __LINE__, __FILE__);

    printf("\n------------------------------------\n");
    printf("Success: All test cases have passed.\n");
    printf("------------------------------------\n\n");

} // end of main

Answer 1

I'll start with the tests.

There's no need to forward-declare the helper functions (and they should both have static linkage).

I don't like the fact that the test program exits on the first failing test. When a change breaks tests, it's often very useful to see which set of tests have been broken.

Tests don't need to be chatty when they succeed. I'm a big fan of producing output only when it's useful, and remaining quiet otherwise. It makes it much easier to focus on what needs to be addressed.

The failure message should start with the file name and line number, each followed by :. That enables standard editor tools (such as compilation-mode) to jump to the source location.

expected is declared as char* but then gets pointed at string literals (i.e. const char*). Declare it with the correct type.

There's a lot of boilerplate within main(). I would use a short macro so that each test can be a single line. That allows more of the tests to be viewed at a time.

---

Now, let's look at the implementation.

There's no need to include <stdio.h> to implement this function.

The boolean argument skip_null_and_empty_input_strings is probably better as a bitmask flags argument, ready for the day when we want to skip null strings but not empty ones.

Why is num_args a signed type? There's really no value in accepting a negative number of strings. I'd expect a size_t for that.

Unless building as C90 is required, there's absolutely no need to declare all variables at the beginning of their scope. And not all variables need to have the entire function as scope. Where possible, declare variables where they can be initialised with values, and make them const if no subsequent change is desired.

The variable arguments shouldn't be written to, so accept them as const char*.

I think it's wrong to return a null pointer just because the strings we've been asked to concatenate are all empty strings (unless perhaps the "ignore empty strings" flag is set). For one thing, that makes it hard to distinguish an error return from a result that should be empty - I would only return null for invalid usage or a runtime error.

&(new_char_array[iica]) is normally written as new_char_array + iica - there's no need to go around the houses with that. Actually, instead of indexing, it's simpler to maintain a write pointer, since we never use the index for anything else.

---

Improved code

#include <assert.h>
#include <stdarg.h>
#include <stdlib.h>
#include <string.h>

/* The description of the function str_join() is in the header file - "str_join.h" */
char *str_join(unsigned int skip_null_and_empty_input_strings,
               const char *delim, size_t num_args, ...)
{
    const size_t delim_len = delim ? strlen(delim) : 0;
    size_t total_len = 1;       /* include the terminating null char */
    size_t num_strings = 0;

    va_list valist;
    va_start(valist, num_args);
    for (size_t i = 0;  i < num_args;  ++i) {
        const char *s = va_arg(valist, const char *);
        const size_t len = s ? strlen(s) : 0;

        if (!len && skip_null_and_empty_input_strings) {
            continue;
        }

        total_len += len;
        if (++num_strings > 1) {
            total_len += delim_len;
        }
    }
    va_end(valist);

    if (!num_strings) {
        return NULL;
    }

    char *const new_char_array = malloc(total_len);
    if (!new_char_array) {
        return new_char_array;
    }
    char *wpos = new_char_array;

    va_start(valist, num_args);
    for (size_t i = 0;  i < num_args;  ++i) {
        const char *s = va_arg(valist, const char *);
        const size_t len = s ? strlen(s) : 0;

        if (!len && skip_null_and_empty_input_strings) {
            continue;
        }

        memcpy(wpos, s, len);
        wpos += len;

        if (num_strings-- > 1) {
            memcpy(wpos, delim, delim_len);
            wpos += delim_len;
        }
    }
    va_end(valist);

    *wpos++ = '\0';
    assert(wpos == new_char_array + total_len);

    return new_char_array;
}

Answer 2

After some long discussion in comments regarding changing the whole concept... here are the main problems:

• Variadic functions are slow, very type-unsafe and should be avoided in general.
• Caller allocation is preferred whenever possible. Partially because it can give faster code, partially because then there's no confusion over who is responsible for clean up (see getline as the perfect example of a problematic API using heap allocation).

I made a brief draft of how you would be able to do this in a conceptually different way, based on chaining multiple strcat calls to each other, which will be more efficient. That is, basically:

char buf [n] = "hello ";
strcat(strcat(buf, "world"), "!!!");

(Though naturally this syntax is a bit icky and a loop with a string array would be preferred.)

However, this assumes that the caller ensures that the buffer is always large enough. One can invent a version of strcat which optionally handles allocation. Either by always doing malloc internally or by getting an allocator function passed by the caller, to enable caller allocation in resource-constrained systems (or even using alloca for stack allocation). A naive implementation might look like this:

typedef void* alloc_t (size_t size);

char* strcata (char* restrict        dst, 
               const char* restrict  src, 
               alloc_t*              alloc)
{
  size_t dst_size = strlen(dst);
  size_t src_size = strlen(src);

  char* new_dst = alloc(dst_size + src_size + 1);
  if(new_dst==NULL) return NULL;

  memcpy(new_dst,          dst, dst_size);
  memcpy(new_dst+dst_size, src, src_size+1); // +1 to also copy null term
  
  return new_dst;
}

#define strcat_alloc(dst,src,alloc) \
  (alloc==NULL) ? strcat( (dst), (src) ) : strcata( (dst), (src), alloc )

This boils down to fairly acceptable and mostly branch-free code, despite the function pointer which can't be inlined, see gcc -O3 x86_64 Linux:

strcata:
        push    r14
        mov     r14, rdi
        push    r13
        mov     r13, rsi
        push    r12
        mov     r12, rdx
        push    rbp
        push    rbx
        call    strlen
        mov     rdi, r13
        mov     rbx, rax
        call    strlen
        mov     rbp, rax
        lea     rdi, [rbx+1+rax]
        call    r12
        mov     r12, rax
        test    rax, rax
        je      .L1
        mov     rdx, rbx
        mov     rsi, r14
        mov     rdi, rax
        call    memcpy
        lea     rdi, [r12+rbx]
        lea     rdx, [rbp+1]
        mov     rsi, r13
        call    memcpy
.L1:
        pop     rbx
        mov     rax, r12
        pop     rbp
        pop     r12
        pop     r13
        pop     r14
        ret

Example of use would be:
https://godbolt.org/z/8WasPnYWv

int main (void)
{
  char buf1[100] = "hello ";

  strcat_alloc(buf1, "world", NULL);
  puts(buf1);
  
  char* buf2 = strcat_alloc(buf1, "!!!", malloc);
  puts(buf2);
  free(buf2);

  char* buf3 = 
  strcat_alloc( 
    strcat_alloc( 
      strcat_alloc("hello ", "world ", malloc),
                   "how's it ", malloc),
                   "going?", malloc);
  // again, a loop would be much prettier than chaining calls like this

  puts(buf3);
  free(buf3);
}

Output:

hello world
hello world!!!
hello world how's it going?