# C-string appender for buffers and strings

Is this C appender safe, and reasonably optimal?

#define REALLOC_BUFFER_SIZE 4096

char *append(char *orig_str, size_t *orig_str_size, const char *append_str) {
// no action required if appending an empty string
if (append_str[0] == 0) {
return orig_str;
}

// does the current char* have enough space?
size_t req_space = strlen(orig_str) + strlen(append_str) + 1; // +1 for NULL byte

// resize if the required space is greater than the original buffer size.
if (req_space > *orig_str_size) {
// prepare the variable with the amount of space we want
size_t allocate = *orig_str_size + req_space + REALLOC_BUFFER_SIZE;

char *new_str = (char *)realloc(orig_str, allocate);
// resize success..
if (new_str == NULL) {
fprintf(stderr, "Couldn't reallocate memory\n");
return NULL;
}

// the realloc didn't fail, here is our new pointer
orig_str = new_str;

// overwrite the free space with NULL's
memset(orig_str + strlen(orig_str), 0, req_space);

// copy the appending text to the string
strcpy(orig_str + strlen(orig_str), append_str);

// modify the original size
*orig_str_size = allocate;

} else {
// append the string
strcpy(orig_str + strlen(orig_str), append_str);
}

return orig_str;
}


Example usage:

char *buffer;
size_t buffer_size = 20;
int i;

buffer = (char *)malloc(sizeof(char) * buffer_size);
strcpy(buffer, "1234567890123456789"); // fill some data prior..

// original string
for (i=1; i < 100; ++i) {
buffer = append(buffer, &buffer_size, "AABBCC");
}

free(x);


Latest changes: GitHub

• In your title, please only state the function/purpose of your code; the body of your post is where the questions should go. – SirPython Mar 4 '15 at 21:46

In addition to what the other answers say and I generally agree with:

Don't call strlen more than once. That's just a waste. Count the length of orig_str and append_str once at the beginning of the function and store them away in const variables. That will also make your initial check whether append_str is empty look cleaner:

const size_t append_len = strlen(append_str);
if (!append_len)
return orig_str;
const size_t orig_len = strlen(orig_str);


Moving the strlen(orig_str) above the conditional would look cleaner in my eyes but it might be a pessimization if append_str is often empty.

Of course, the most efficient way would be if you could pass orig_len as an additional input parameter. Or you could use a

struct string_buffer
{
size_t length;
size_t capacity;
char * data;
};


that encapsulates all the required info and change your function's signature to

ssize_t
append_string(struct string_buffer * buffer, const char * appendix);


where the function could return the number of bytes successfully appended or –1 on error.

My second remark would be that you are growing your buffer by a constant amount each time you reallocate. It should really grow exponentially unless you really know that linear growth will be preferable in your specific use-case, which I find hard to imagine how that could be.

Concerning your comments: there is no NULL byte. NULL is a macro constant for the null pointer. The null byte is abbreviated as NUL in the ASCII character set. I would also find it helpful to have your function's semantics exactly documented in a comment. For example, should the user free the buffer if it fails and returns NULL? Will the existing data be modified in that case? I can try answering those questions by reading your implementation but that doesn't allow me to distinguish a bug in your code from a different valid design decision.

• strcat(orig_str, append_str) looks cleaner than a strcpy/strlen combo.

• memset is pointless. You need only 1 terminating zero, and it is taken care of by string routines.

• The responsibilities of if (req_space > *orig_str_size) and a corresponding else clauses overlap. The only task of the if clause should be string reallocation. Copying needs to be performed in any case. Consider instead

if (req_space > *orig_str_size) {
allocate = ...;
char *new_str = realloc(orig_str, allocate);
if (new_str == NULL) {
handle_error;
}
*orig_str_size = allocate;
orig_str = new_str;
}
strcat(orig_str, append_str);

• fprintf doesn't belong to a such low level routine. Returning an error condition is enough. It is up to the caller to print or not to print.

This line:

char *new_str = (char *)realloc(orig_str, allocate);


has a problem:

1. In C, do not cast the returned value from realloc (and family).

In this line, from the usage example:

buffer = (char *)malloc(sizeof(char) * buffer_size);


there are a couple of problems:

1. In C, do not cast the returned value from malloc (and family).

2. Always check the returned value (!= NULL) to assure the operation was successful.

Another thing about this function that bugs me. If a realloc happens, the orig_str is invalidated. That goes against my expectation of parameters passed to functions. Instead of returning the new char *, how about changing the pointer to char ** pOrig_str so it can be modified in-place?