strcat_new() function, not present in standard C library

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

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

Attribution
Source : Link , Question Author : Amit , Answer Author : chux – Reinstate Monica

Leave a Comment