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 #include
s.
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