11
\$\begingroup\$

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");

}
\$\endgroup\$
8
  • \$\begingroup\$ Please do not edit the question, especially the code, after an answer has been posted. Changing the question may cause answer invalidation. Everyone needs to be able to see what the reviewer was referring to. What to do after the question has been answered. \$\endgroup\$
    – pacmaninbw
    Nov 30 '21 at 13:26
  • 15
    \$\begingroup\$ By analogy with similar functions in many other languages, consider calling it strjoin rather than strcat_new, which makes one think it's similar to strcat, when it's really very different. \$\endgroup\$
    – jcaron
    Nov 30 '21 at 15:18
  • \$\begingroup\$ @pacmaninbw, the existing code had a bug. I fixed that bug and updated the code. Please tell me how should I post the code that doesn't have the bug. Shall I post a new question? \$\endgroup\$
    – Amit
    Dec 2 '21 at 5:57
  • \$\begingroup\$ Also, even though varargs is practical for "manual" inputs, this is way more likely to be called with variable inputs decided at runtime (e.g. results of a database or API query). Providing an interface which takes an array rather than varargs is probably necessary. That one would be strjoin, and a varargs-based interface would be called vstrjoin (by analogy with sprintf/vsprintf etc.). Evidently, one should just call the other after conversion of the arguments. We'll leave it as an exercise to determine which one calls which :-) \$\endgroup\$
    – jcaron
    Dec 2 '21 at 11:05
  • \$\begingroup\$ To post any corrections to the code the best thing would be to ask a follow up question. A follow up question will have a link back to this question. You also might want to accept one of the three answers. \$\endgroup\$
    – pacmaninbw
    Dec 2 '21 at 15:57
19
\$\begingroup\$

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>
<>
<>
\$\endgroup\$
8
  • \$\begingroup\$ Comments are not for extended discussion; this conversation has been moved to chat. \$\endgroup\$ Dec 2 '21 at 2:51
  • \$\begingroup\$ re: naming: strcatdup or strdupcat occur to me; strdup mallocs a copy of a string; strcat concatenates. Or jcaron's suggestion of strjoin is appealing, especially given the delim arg. \$\endgroup\$ Dec 2 '21 at 9:23
  • 2
    \$\begingroup\$ @Peter, all those names are in the "reserved for library expansion" set. User code should not define external identifiers beginning with str (or any identifier of that form when <stdlib.h> or <string.h> are included), unless the next character is something other than a lower-case letter. Perhaps str_join? \$\endgroup\$ Dec 2 '21 at 11:10
  • 1
    \$\begingroup\$ See Should I use #include in headers? on SO for a discussion of header guards and 'idempotence', 'self-containment', and 'minimalism'. \$\endgroup\$ Dec 2 '21 at 16:59
  • \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ Dec 30 '21 at 12:39
9
\$\begingroup\$

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)
        ;
}
\$\endgroup\$
13
  • 1
    \$\begingroup\$ "Provide a constant for that purpose" No, really not. (char*)0 is so much more obvious and better to remember. \$\endgroup\$ Nov 30 '21 at 15:23
  • 3
    \$\begingroup\$ @Joshua Maybe in crazy pre-standard headers? And anyway, size_t is a semantic type seeing ubiquitous use, not a one-off name to hide the standard sentinel. \$\endgroup\$ Nov 30 '21 at 20:18
  • 1
    \$\begingroup\$ @Deduplicator: Well if you did compile on a platform with an int size_t your comparison between signed and unsigned warnings would trigger. \$\endgroup\$
    – Joshua
    Nov 30 '21 at 20:19
  • 1
    \$\begingroup\$ I concur with the suggestion to terminate the argument list, instead of passing a counter. \$\endgroup\$ Dec 1 '21 at 8:12
  • 2
    \$\begingroup\$ @Peter, (almost) Standard reference: ISO/IEC 9899:201x committee draft section 7.19.2: "size_t which is the unsigned integer type of the result of the sizeof operator;" \$\endgroup\$ Dec 2 '21 at 10:51
4
\$\begingroup\$

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;
}
\$\endgroup\$
2
  • \$\begingroup\$ C99 variable length arrays are great but I don' use them because if someone is using C89 then it will be a lot of work for that person to make my program work in C89. Removing VLA will involve lot of work because lot of code will need to be changed. \$\endgroup\$
    – Amit
    Dec 30 '21 at 15:42
  • \$\begingroup\$ That's the reason I prefer the second option, without VLAs. Imho other C99 features, especially mixing declarations and instructions are fair game, as I don't know anyone which didn't add that as an extension. \$\endgroup\$ Dec 30 '21 at 15:45

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.