Concatenate variable number of strings

Question

I am learning C on my own. This is not an assignment. I am trying to implement a function that takes a variable numbers of strings and concatenate them in a new string. It works, but I feel it is not elegant and would like to improve it.

For instance, I don't like how I am using n nor having to decrement argc twice, and I don't like having to use both length and total_length. On a more general level, I would like to know if there is a way to parse the variable arguments only once, although it does not seem to be possible unless allocating a fixed number of character or calling malloc multiple times (which I want to avoid).

/* 
 * concat_strings - concatenate a variable number of strings
 * usage:
 * const *message = concat_strings(4, "Error running", argv[0], " - ", "Action create: needs name and email");
 */
char *concat_strings(int argc, ...)
{
    va_list ap;

    /* calculate the total number of characters in all the strings */
    va_start(ap, argc);
    int n = argc;
    int l = 0;
    while(n--)
        l += strlen(va_arg(ap, char*));
    va_end(ap);

    /* allocate memory */
    const size_t sizeof_char = sizeof(char);
    char *joined = malloc(sizeof_char * (l + 1));

    /* copy strings to destination */
    va_start(ap, argc);
    size_t length = 0;
    size_t total_length = 0;
    char *s;
    while(argc--) {
        s = va_arg(ap, char*);
        memcpy(joined + total_length, s, sizeof_char * (length = strlen(s)));
        total_length += length;
    }
    *(joined + total_length) = '\0';
    va_end(ap);

    return joined;
}

Answer 1

Calling strlen() twice on each string is wasteful in time.

Current code make 3 trips down each string, once for length, once for copying, once to determine new length.

    l += strlen(va_arg(ap, char*));
    ...
    memcpy(joined + total_length, s, sizeof_char * (length = strlen(s)));
    total_length += length;

The following runs down each string twice, once for length determination and a second for copying. It "parses" the variable arguments only once, but then does need to save those results somewhere.

char* concat_n(size_t n, ...) {
  va_list ap;
  size_t size = 1;

  // Allocate memory or for small `n`, use VLA or fixed array.
  struct {
    size_t len;
    const char *ptr;
  } str[n];

  va_start(ap, n);
  for (size_t i = 0; i < n; i++) {
    str[i].ptr = va_arg(ap, const char *);
    str[i].len = strlen(str[i].ptr);
    if (str[i].len > SIZE_MAX - size) {  // Detect overflow in length
      va_end(ap);
      return NULL;
    }
    size += str[i].len;
  }
  va_end(ap);

  char *dest = malloc(size);
  if (dest) {
    char *p = dest;
    for (size_t i = 0; i < n; i++) {
      memcpy(p, str[i].ptr, str[i].len);
      p += str[i].len;
    }
    *p = '\0';
  }
  return dest;
}

---

An alternative to saving the lengths and recomputing strlen() is to use some non-standard function like char *strpcpy() which copies and returns the pointer to the null character. As if this is faster than other approaches, YMMV. IMO, memcpy() is the best to speed code along and since the length was compute prior, take advantage of that even if managing the saving of those prior results is a minor extra burden.

---

int is not the best type for array index nor size math. size_t is the Goldilocks type, neither too wide, nor too narrow.

---

sizeof_char appears artificial. Alternatives:

// char *joined = malloc(sizeof_char * (l + 1));
char *joined = malloc(l + 1);
// or
char *joined = malloc(sizeof *joined * (l + 1));

---

Defensive programming. What happens if user passed a negative value for argc? (Answer: a very long loop).

// Problem code when `argc < 0`
char *concat_strings(int argc, ...) {
  ...
  int n = argc;
  while(n--) { 

Suggest either size_t or > 0

char *concat_strings(size_t argc, ...) {
  ...
  size_t n = argc;
  while(n--) { 

or

char *concat_strings(int argc, ...) {
  ...
  int n = argc;
  while(n-- > 0) { 

---

Check allocation

char *joined = malloc(l + 1);
if (joined == NULL) return NULL;

---

argc, though not a reserved name, has special meaning. Suggest a different variable name like string_count