str_join() function, not present in the standard C library

Question 1

This function str_join() is an updated version of my earlier function strcat_new() — found here: strcat_new() function, not present in the Standard C library.

The older version strcat_new() had a bug that is fixed in this version of str_join().

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

str_join.c


#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>
#include "str_join.h"
/* 
 * str_join:
 *
 * 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:
 * str_join 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 *str_join(const 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;
 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 (!temp) continue;
 total_len = total_len + strlen(temp);
 num_delim_to_concat = num_delim_to_concat + 1;
 }
 va_end(valist);
 if (total_len == 0)
 return NULL;
 total_len = total_len + (num_delim_to_concat) * (delim_len);
 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 (num_delim_to_concat > 0) {
 for (j = 0; j < delim_len; j++) {
 new_char_array[iica] = delim[j];
 iica++;
 }
 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 _STRCAT_NEW_H_
#define _STRCAT_NEW_H_
/* 
 * str_join:
 *
 * 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:
 * str_join 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 *str_join(const char *delim, long num_args, ...);
#endif

test_str_join.c


#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include "str_join.h"
int main(void)
{
 char *a = str_join(";:?", 4, "abc", "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join(NULL, 3, "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join(NULL, 0, "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join(NULL, -1, "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join("=", 4, "abc", "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join("{(=%$^%^&&(&)}", 4, "abc", "123", "xyz", "455");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join(";;;", 4, "abc", NULL, NULL, NULL);
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join(";;;", 4, "abc", NULL, NULL, "cde");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, strlen(a));
 free(a);
 a = str_join(NULL, 4, NULL, "fdr", NULL, NULL);
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join("-------", 10, "START", "hfjkdhf78578695590", "nvmcnkjfhgkjfh^&*^&*", "7856475947584", "!@^&^%#*&(*$&(&$(*", "j", "4", "khfkjhgiury6985768956uirhtukrhgkhfk", "jfjlkjgk65876895769576895khgkjfkkhkjhjG%^&%#*@^*@^$*&^#&$*#$#*$&#$&$(*#", "END");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join(NULL, 1, NULL, "fdr", NULL, NULL);
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join(";", 4, NULL, NULL, NULL, "xyz");
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join(";;;", 4, NULL, NULL, NULL, NULL);
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 a = str_join(NULL, 4, NULL, NULL, NULL, NULL);
 printf("\n");
 printf("a = %s, strlen(a) = %lu\n", a, a?strlen(a):(unsigned long)(-1));
 free(a);
 printf("\n");
}

Question 2

Just a little tip for improvement: create a more generic version of the function - one that accepts a malloc-like function pointer. Then the str_join itself would just call it passing malloc as the function pointer. This will allow you to test the inner function against a custom malloc implementation and you can actually test whether the right amount of memory is allocated.

Question 3

(If you intentionally restrict the code to C89, please tag accordingly.)

Question 4

@greybeard Must be at least C99, as no return 0; in main().

Question 5

What's wrong with using snprintf() to join strings?

Question 6

No test reference

There are no tests in test_str_join.c, just code being exercised. How is the test to detect if output is correct? If by visual inspection, including comments of expected output would at least be a good start.

Undefined behavior printing a null pointer

Example: last test may print a = (null), strlen(a) = 18446744073709551615 or crash or ... It is UB.

NULL is not safe as a ... argument

The type of NULL is void *, int, long, some integer type. It is not safe to fetch via va_arg(valist, char *).

Better test code would use (void *) NULL, (void *) 0.

Wrong type

Types used for string length are as if long and size_t were the similar types. size_t is the type returned by strlen() and the type used in allocations. size_t is the Goldilocks type for array indexing, neither too narrow nor too wide. long is signed, size_t is unsigned. LONG_MAX may be less than or more than (or even the same as) SIZE_MAX. long usage here remains questionable.

If you want to remain with long, use len = (long) strlen(temp); to quiet warnings like :

warning: conversion to 'size_t' from 'long int' may change the sign of the result [-Wsign-conversion]

Why delimiter after last?

Goal is "after every string, the 'delim' string is concatenated". Then later says "separated by 'delim' string". This code apparently only delimits between strings. Comments need revision for consistency.

Zero num_args

I still maintain a return of allocated "" is expected for char *str_join(",", 0) rather than NULL of this code.

Repeated comments

Parameters: and Description: are repeated in the *.c and *.h files. Once is enough - best in the .h file. Consider code maintenance is more expensive that original code writing and maintaining same comments in 2 files is additional unnecessary work - likely to diverge over time.

Print formed string with sentinels

Easier to identify unexpected leading and trailing white-space.

// printf("a = %s ...
printf("a = \"%s\" ...

Simplify

To code a (unsigned long)(-1), easy to use -1UL.

Define when needed

Rather than define an object far from its use and at a higher scope than needed:

//char *temp = NULL;
//... 8 lines
//for (i = 0; i < num_args; i++) {
// temp = va_arg(valist, char *);
// if (!temp) continue;
// total_len = total_len + strlen(temp);
for (i = 0; i < num_args; i++) {
 char *temp = va_arg(valist, char *);
 if (!temp) continue;
 total_len = total_len + strlen(temp);

Minor: avoid !

I have found less misunderstandings when !, != are avoided. Consider alternatives.

// if (!temp) continue;
if (temp == NULL) continue;

2nd posting without finishing first.

Consider wrapping up earlier related questions before posing an updated question.

unfinished before new business.

Question 7

Maybe add: Contradictory description: Is delim inserted between any two, or after every single argument...

Question 8

@Deduplicator Nice alternative way to say it. Issue brought up before. Unclear if OP wants to address it.

Question 9

I will fix the description. Basically, the delim is between valid (non-null and non-empty) strings (arguments). I also need to fix the empty string(s) case - if the user passes empty string(s) then these empty string(s) will not be concatenated/joined and will be ignored.

Question 10

Chux, on your comment: Undefined behavior printing a null pointer - how will it crash? How is it unbounded?

Question 11

@Amit, re "Undefined behavior printing a null pointer - how will it crash? How is it unbounded?" to attempt to use "%s" with an invalid pointer -- it is undefined (UB), there is no prescribed way it will crash - it is not defined. It might "work", it might crash in various ways. If you still want to print something reliable when s == NULL, instead of printf("%s\n", s); use printf("%s\n", s ? s : "null");

Question 12

The macro name _STRCAT_NEW_H_ is an identifier reserved for any purpose by the implementation. Do not use identifiers beginning with _ in your own code.

Always include your defining header first in your implementation file, before standard headers. This can help identify accidental dependencies (i.e. creating a header that's not self-sufficient).

Many of the variables have much larger scope than necessary. Reduce scope, and delay declaration to where the variable can be initialised.

Avoid using signed types where unsigned makes more sense. For example, num_args should be a size_t, or possibly unsigned given that we should not be passing 65536 or more arguments in a function call. Certainly the "length" accumulators should be size_t.

The local variables for the argument strings should be const char*, because callers should be able to pass non-writable strings.

Don't return a null pointer when the result is an empty string - that just makes life difficult for the caller, and makes it impossible to tell whether the function was successful or not. Be careful that num_delim_to_concat doesn't remain at -1 in that case, though.

Consider splitting out a function that accepts a va_list argument (ala vsprintf()) so that calling code is able to forward argument lists. You'll find this useful for the tests, too.

Use standard functions (strcat/strcpy/memcpy) for writing to the output string, instead of hand-rolling loops. A decent optimising compiler will likely generate the same code for both, but at less risk of programmer error.

Prefer idiomatic increment - e.g. --num_delim_to_concat rather than num_delim_to_concat = num_delim_to_concat - 1, and total_len += num_delim_to_concat * delim_len rather than total_len = total_len + (num_delim_to_concat) * (delim_len). Writing these out longhand leaves the reader wondering what's going on.

The tests are repetitive, which suggests that a helper function may be valuable. Also, instead of being self-checking, they require manual inspection, which is time-consuming and error-prone. Rewrite so that the exit status indicates whether the tests passed or not.

Tests pass NULL as a vararg parameter. This needs to be cast to char* to be safely transferred.

Tests should help demonstrate how the code is to be used. Therefore, they should test whether a null pointer was returned, rather than assuming a valid object pointer.

Question 13

Toby, about Many of the variables have much larger scope than necessary: I don't like "define/declare variables when needed". This way my variables would be scattered all across the function. I want all my variables at the top - I feel comfortable doing this. Scattering my variables all across the function doesn't make me feel comfortable. All the places I worked (I worked mostly in C), there was no guideline to follow "define/declare when needed". I have seen lots and lots of active C production code which does not follow "define/declare when needed" approach.

Question 14

Toby, size_t vs long is debatable/personal preference. I like long instead of size_t. I have no problem if someone doesn't want to use my code because of this.

Question 15

Another downside to declaring all variables at start of function scope is the risk of use before assignment. It's possible that the the "lots of production code" was written prior to 1999, when declarations couldn't be intermixed with statements. Using long instead of size_t is just wrong (both in range and signedness), and the resulting warnings can obscure other problems in your code (and if you're not getting warnings, you really should be enabling them).

Question 16

There is no risk of use before assignment because variables should be initialized at the time of declaration.

Question 17

The lots of production code that I saw (that didn't use define/declare when needed) was as late as 2020.

chux chux 36.4k2 gold badges43 silver badges96 bronze badges · Answer 1 · 2021-12-07 19:38:22Z