strcat_new() function, not present in standard C library

Question

strcat_new() function, not present in standard C library.

Syntax: char *strcat_new(char *delim, long num_args, ...);

The code is below. Can someone please do the code review?

UPDATE: This version of strcat_new() has a bug. This bug has been fixed in the newer version. The newer version is called str_join(). You can find the newer version here: str_join() function, not present in standard C library

---

strcat_new.c

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

#include "strcat_new.h"

/* 
 * strcat_new:
 *
 * Parameters:
 *      num_args: number of variable arguments that are passed to this function
 *                excluding the 'delim' string.
 *      ...: Variable number of pointers to character arrays.
 *
 * Description:
 *      strcat_new concatenates all the strings/character arrays passed to it. If
 *      'delim' is not NULL then after every string, the 'delim' string is concatenated.
 *      It 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 separated by 'delim' string
 *      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.
 */
char *strcat_new(char *delim, long num_args, ...)
{

    va_list valist;
    long i = 0;
    long j = 0;
    long iica = 0; // iica - index into character array
    long len = 0;
    long delim_len = 0;
    long total_len = 0;
    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 (!temp) continue;
        total_len = total_len + strlen(temp) + delim_len;
    }
    va_end(valist);

    total_len = total_len - delim_len; // remove the last delimiter

    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 (!temp) continue;
        len = strlen(temp);
        for (j = 0; j < len; j++) {
            new_char_array[iica] = temp[j];
            iica++;
        }
        if (i < (num_args - 1)) {
            for (j = 0; j < delim_len; j++) {
                new_char_array[iica] = delim[j];
                iica++;
            }
        }
    }
    va_end(valist);

    new_char_array[iica] = 0;

    return new_char_array;

} // end of strcat_new

---

strcat_new.h

#ifndef _STRCAT_NEW_H_
#define _STRCAT_NEW_H_

/* 
 * strcat_new:
 *
 * Parameters:
 *      num_args: number of variable arguments that are passed to this function
 *                excluding the 'delim' string.
 *      ...: Variable number of pointers to character arrays.
 *
 * Description:
 *      strcat_new concatenates all the strings/character arrays passed to it. If
 *      'delim' is not NULL then after every string, the 'delim' string is concatenated.
 *      It 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 separated by 'delim' string
 *      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.
 */
char *strcat_new(char *delim, long num_args, ...);

#endif

---

test_strcat_new.c

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

#include "strcat_new.h"

int main(void)
{

    char *a = strcat_new(";:?", 4, "abc", "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
    free(a);

    a = strcat_new(NULL, 3, "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
    free(a);

    a = strcat_new(NULL, 0, "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):0);
    free(a);

    a = strcat_new(NULL, -1, "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):0);
    free(a);

    a = strcat_new("=", 4, "abc", "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
    free(a);

    a = strcat_new("{(=%$^%^&&(&)}", 4, "abc", "123", "xyz", "455");
    printf("\n");
    printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
    free(a);

    printf("\n");

}

Answer 1

Neat and well formatted

Design

Rather than "then after every string, the 'delim' string is concatenated." (that sounds like n delimiters), I would expect between every string. between, or the like, matches code (n - 1 delimiters).

Design: num_args == 0

Consider allowing this corner case.

// if (num_args <= 0) return NULL;
if (num_args < 0) return NULL;  // Do not error on 0

Other code may need to change too.

Use const

Allow const string pointer as delim.

// char *strcat_new(char *delim, long num_args, ...)
char *strcat_new(const char *delim, long num_args, ...)

Why long?

size_t num_args (Goldilocks type for array sizing and indexing) or int num_args (the most common type) would make more sense.

For pedantic code, could use uintmax_t len to sum the length needs and make sure it is not more than SIZE_MAX.

"strcat_new.h" first

In strcat_new.c, (not other *.c), put #include "strcat_new.h" first, before <> includes, to insure "strcat_new.h" compiles on its own without <> includes in the .c files.

So, an #include "xxx.h" should work without requiring a prior include. To test that, the xxx.c should first #include "xxx.h" without previous #includes.

Redundant description

Reduce maintenance. In *.c use

/* 
 * strcat_new:
 *
 * See strcat_new.h
 */

Define and assign

Define when needed.

// char *new_char_array = NULL;
...
// new_char_array = malloc(total_len);

...
char *new_char_array = malloc(total_len);

memcpy() vs. user code

Rather than user loops, consider memcpy(), strcpy().

    for (j = 0; j < len; j++) {
        new_char_array[iica] = temp[j];
        iica++;
    }

    // vs

    memcpy(new_char_array + iica, temp, len);
    iica += len;  

For long strings, likely faster.

Enable all warnings

long delim_len = 0; delim_len = strlen(delim); implies all warnings not enabled as that is a change of sign-ness with no cast.

1 strlen() per arg

[See code far below for alternate idea.]

I would consider saving the strlen(temp) rather than calling twice.

It is more fuss for short concatenations, but a savings for long ones. It is for long cases that such micro-optimizations are useful.

Minor: Name

Perhaps alloc_strcat() rather than strcat_new(). More C like than C++?

str...() may collide with future library. "Function names that begin with str, mem, or wcs and a lowercase letter may be added to the declarations in the <string.h> header."

Bug

As is, when temp == NULL, the code skips to print the next delimiter. It should print delimiters selectively. Consider strcat_new(",", 3, "123", "xyz", (char*)0); would print a delimiter at the end.

printf("<%s>\n", strcat_new(",", 3, "123", "xyz", (char*)0));
// Output
<123,xyz,>
//      ^ extra delimiter.

A better delim print would be

// Pseudo code
delim_printing_needed = false;
for each arg
  if (delim_printing_needed) print_delim
  if (arg is not null) {
    print arg
    delim_printing_needed = true;
  }
}

Pedantic: Consider restrict

Let compiler know delim is not changed by access in other strange ways. Allows for some optimizations.

char *strcat_new(const char * restrict delim, long num_args, ...)

strlen() returns size_t

// printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
printf("a = %s, strlen(a) = %zu\n", a, strlen(a));
//                           ^

Consider sentinels

Easier to see white-space troubles.

// printf("a = %s, strlen(a) = %zu\n", a, strlen(a));
printf("a = \"%s\", strlen(a) = %zu\n", a, strlen(a));
//           ^   ^

Hidden problem - null pointer vs NULL

Code has if (!temp) continue; implying a null pointer may be passed as one of the ... arguments. Test code does not test this.

Using NULL, a null pointer constant, as a ... argument is problematic as the type of NULL may differ in size from a null pointer - strange as that sounds.

What is needed is a null pointer, like ((void *) NULL). Here is may be useful to form a null pointer to be used in skipping like #define NULL_CHAR_POINTER ((char *) 0).

Using NULL as the delim argument is not a problem as it is converted to a char *.

---

[Edit]

Looked fun to try to incorporate these ideas.

Saving the arg lengths not really needed to maintain only 2 passes through each arg (vs. OP's 3 passes: 2x strlen() and for loop).

#include "alloc_concat.h"
#include <stdbool.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>

// Copy string and return string end
// If stpcpy available, use it, else ...
static char *my_stpcpy(char *restrict dest, const char *restrict src) {
  while ((*dest = *src) != '\0') {
    dest++;
    src++;
  }
  return dest;
}


/*
 * Since it is so important to free the returned pointed,
 * I like the function name to echo that.
 */
char *concat_alloc(const char *delim, size_t num_args, ...) {
  uintmax_t total_sz = 1;
  const size_t delim_len = delim ? strlen(delim) : 0;

  // Walk args, summing space needs.
  va_list valist;
  va_start(valist, num_args);
  bool delim_needed = false;
  for (size_t i = 0; i < num_args; i++) {
    char *arg = va_arg(valist, char *);
    if (arg) {
      if (delim_needed) {
        total_sz += delim_len;
      }
      total_sz += strlen(arg);
      delim_needed = true;
    }
  }
  va_end(valist);

  char *concat = total_sz > SIZE_MAX ? NULL : malloc((size_t) total_sz);
  if (concat == NULL) {
     return NULL;
  }
  char *s = concat;

  // Walk args, appending  delim and arg.
  va_start(valist, num_args);
  delim_needed = false;
  for (size_t i = 0; i < num_args; i++) {
    char *arg = va_arg(valist, char *);
    if (arg) {
      if (delim_needed) {
        s = my_stpcpy(s, delim);
      }
      s = my_stpcpy(s, arg);
      delim_needed = true;
    }
  }
  va_end(valist);
  *s = '\0';  // In case my_stpcpy() never called.

  return concat;
}

Some tests. (Not very extensive)

#include<stdio.h>

int main() {
  char *s;
  s = concat_alloc(", ", 3, "1", "2", "3"); printf("<%s>\n", s); free(s);
  s = concat_alloc(", ", 3, (char*)0, "2", "3"); printf("<%s>\n", s); free(s);
  s = concat_alloc(", ", 3, "1", (char*)0, "3"); printf("<%s>\n", s); free(s);
  s = concat_alloc(", ", 3, (char*)0, "2", "3"); printf("<%s>\n", s); free(s);
  s = concat_alloc(", ", 3, (char*)0, (char*)0, (char*)0); printf("<%s>\n", s); free(s);
  s = concat_alloc(", ", 0); printf("<%s>\n", s); free(s);
}

Output

<1, 2, 3>
<2, 3>
<1, 3>
<2, 3>
<>
<>

Answer 2

Reserved identifiers

The include-guard _STRCAT_NEW_H_ is one of the names reserved for the implementation, as it begins with underscore and an upper-case letter.

As a general guide, never begin names with underscore.

The function name beginning with str is reserved for standard library extension, so choose a different name there, too.

Interface

Consider using a sentinel (such as null pointer) to mark the last argument, rather than requiring the programmer to keep the count argument up to date. To retain the behaviour of silently ignoring arguments which are null pointers, make the sentinel a distinct pointer value:

extern const char end_string_args[] = { 0 };

Implementation

As chux's review says, make sure you have warnings enabled for conversions between signed and unsigned types. There are plenty of problems you can find and fix that way, before asking humans to review the code.

I don't think this function requires any signed values at all.

Include the defining header before standard library headers. That helps identify any includes we may be missing.

Bug

Fix the bug where an extra delimiter is printed when the last string to be concatenated is a null pointer. Write the test case first, and check using Valgrind or other tool that we don't overwrite memory we shouldn't.

Tests

The tests should set a reasonable example for calling code to follow, and consider that a null pointer may be returned.

We're missing tests for important parts of the functionality. In particular, we should test that null-pointer arguments ((char*)0) are ignored - and these tests should include nulls as first, middle, last and only arguments. We should also test the case where there are no strings to concatenate.

We could improve the tests by making them self-checking, so that the exit status indicates the correctness of the code, rather than relying on manual inspection. When writing these tests, you'll probably find a need for a version that accepts a va_list argument (similar to vsprintf() and family in the standard library), so split the implementation accordingly.

---

Modified code

#ifndef ALLOC_CONCAT_H
#define ALLOC_CONCAT_H

#include <stdarg.h>

/*
 alloc_concat:

 Parameters:
      delim: separator to be printed between string arguments
      (omitted for null args).
      ...: Variable number of pointers to character arrays, terminated
      with end_string_args.

 Description:
      alloc_concat concatenates all the strings/character arrays passed
      to it.  If 'delim' is not null then after every string, the 'delim'
      string is concatenated.  It allocates a new character array whose
      size is large enough for the result.

      If memory allocation fails then a null pointer is returned.

      It is the responsibility of the caller to free the allocated
      memory.
 */
char *alloc_concat(const char *delim, ...);

char *alloc_concat_v(const char *delim, va_list args);

extern const char end_string_args[];

#endif

#include "alloc_concat.h"

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

const char end_string_args[] = { 0 };

char *alloc_concat_v(const char *delim, va_list args)
{
    if (!delim) {
        /* null delim means empty delim */
        delim = "";
    }
    const size_t delim_len = strlen(delim);
    size_t total_len = 0;
    size_t argc = 0;        /* count of non-null arguments */

    va_list args_m;
    va_copy(args_m, args);
    for (const char *s; (s = va_arg(args_m, const char*)) != end_string_args; ) {
        if (s) {
            total_len += strlen(s);
            ++argc;
        }
    }
    va_end(args_m);

    if (!argc) {
        /* empty string */
        return calloc(1, 1);
    }

    /* Add space for delimiters and terminator */
    total_len += (argc - 1) * delim_len + 1;

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

    char *p = new_char_array;
    while (argc) {
        const char *const s = va_arg(args, const char*);
        if (!s) { continue; }
        const size_t len = strlen(s);
        memcpy(p, s, len);
        p += len;
        /* if it's not the last non-null argument, append the separator */
        if (--argc) {
            memcpy(p, delim, delim_len);
            p += delim_len;
        }
    }
    va_end(args);
    *p = '\0';

    return new_char_array;
}

char *alloc_concat(const char *delim, ...)
{
    va_list args;
    va_start(args, delim);
    char *result = alloc_concat_v(delim, args);
    va_end(args);
    return result;
}

#include "alloc_concat.h"

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

/* Test helper */
static int test_concat(const char *file, int line,
                       const char *inputs,
                       const char *expected,
                       const char *sep, ...)
{
    va_list args;
    va_start(args, sep);
    char *result = alloc_concat_v(sep, args);
    va_end(args);

    if (!sep) {
        /* ensure it's printable in diagnostics */
        sep = "(null)";
    }
    if (!result) {
        fprintf(stderr,
                "%s:%d:warning: got null pointer\n"
                "%s:%d:note: from \"%s\", %s\n",
                file, line,
                file, line, sep, inputs);
        return 1;
    }
    if (!strcmp(expected, result)) {
        /* passed */
        free(result);
        return 0;
    }
    fprintf(stderr,
            "%s:%d:error: got \"%s\" where  \"%s\" expected\n"
            "%s:%d:note: from \"%s\", %s\n",
            file, line, result, expected,
            file, line, sep, inputs);
    free(result);
    return 1;
}

#define TEST_CONCAT(expected, sep, ...) \
    test_concat(__FILE__, __LINE__, #__VA_ARGS__, expected, sep, __VA_ARGS__)


int main(void)
{
    return 0 !=
        + TEST_CONCAT("", NULL, end_string_args)
        + TEST_CONCAT("", "+", end_string_args)
        + TEST_CONCAT("", NULL, (char*)0, end_string_args)
        + TEST_CONCAT("", "\\", (char*)0, end_string_args)
        + TEST_CONCAT("a", NULL, "a", end_string_args)
        + TEST_CONCAT("ab", NULL, "a", "b", end_string_args)
        + TEST_CONCAT("ab", NULL, (char*)0, "a", "b", end_string_args)
        + TEST_CONCAT("ab", NULL, "a", (char*)0, "b", end_string_args)
        + TEST_CONCAT("ab", NULL, "a", "b", (char*)0, end_string_args)
        + TEST_CONCAT("a%-bb", "%-", (char*)0, "a", "bb", end_string_args)
        + TEST_CONCAT("a%-bb", "%-", "a", (char*)0, "bb", end_string_args)
        + TEST_CONCAT("a%-bb", "%-", "a", "bb", (char*)0, end_string_args)
        ;
}

Answer 3

Using C99 Variable Length Arrays to save calculated string-lengths, and fixing all the other problems chux identified, your code can be made far more efficient:

char* astrcat(const char* restrict delim, size_t num_args, ...) {
    if (!num_args)
        return calloc(1, 1);

    size_t lengths[num_args];
    const size_t ndelim = delim ? strlen(delim) : 0;
    size_t total = (num_args - 1) * ndelim;

    va_list args;
    va_start(args, num_args);
    for (size_t i = 0; i < num_args; ++i) {
        lengths[i] = strlen(va_arg(args, const char*));
        total += lengths[i];
    }
    va_end(args);

    char* const r = malloc(total + 1);
    if (!r)
        return r;

    char* p = r;
    va_start(args, num_args);
    for (size_t i = 0;; ++i) {
        memcpy(p, va_arg(args, const char*), lengths[i]);
        p += lengths[i];
        if (i == num_args)
            break;
        memcpy(p, delim, ndelim);
        p += ndelim;
    }
    *p = 0;
    va_end(args);

    return r;
}

If you don't want (or can't) use VLA, you can use simple recursion (this version also changed to use a sentinel instead of a manual count, using sentinel above or count below left as an exercise for the reader):

static char* vastrcat_impl(const char* restrict delim, size_t ndelim,
        size_t prefix, va_list args) {
    const char* p = va_arg(args, const char*);
    if (!p) {
        char* r = malloc(prefix + 1);
        if (r)
            *(r += prefix) = 0;
        return r;
    }
    size_t n = strlen(p);
    char* r = vastrcat_impl(delim, ndelim, prefix + n + ndelim, args);
    if (!r)
        return r;
    memcpy(r -= n, p, n);
    memcpy(r -= ndelim, delim, ndelim);
    return r;
}

char* vastrcat(const char* restrict delim, va_list args) {
    const char* p = va_arg(args, const char*);
    if (!p)
        return calloc(1, 1);
    size_t n = strlen(p);
    size_t ndelim = delim ? strlen(delim) : 0;
    char* r = vastrcat_impl(delim, ndelim, n, args);
    memcpy(r -= n, p, n);
    return r;
}

char* astrcat(const char* restrict delim, ...) {
    va_list args;
    va_start(args, delim);
    char* r = vastrcat(delim, args);
    va_end(args);
    return r;
}