C-string appender for buffers and strings

Question

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
– SirPython, Commented Mar 4, 2015 at 21:46

5gon12eder · Accepted Answer · 2015-03-05 01:35:59Z

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.

vnp · Accepted Answer · 2015-03-05 00:37:00Z

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.

glampert · Accepted Answer · 2015-03-05 03:06:51Z

3

This line:

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

has a problem:

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:

In C, do not cast the returned value from malloc (and family).
Always check the returned value (!= NULL) to assure the operation was successful.

edited Mar 5, 2015 at 3:06

glampert

17.3k4 gold badges31 silver badges89 bronze badges

answered Mar 4, 2015 at 23:07

user3629249

2,92812 silver badges10 bronze badges

Add a comment |

Snowbody · Accepted Answer · 2015-03-09 20:21:08Z

0

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?

answered Mar 9, 2015 at 20:21

Snowbody

8,69225 silver badges50 bronze badges

Add a comment |

Stack Exchange Network

C-string appender for buffers and strings

4 Answers 4

You must log in to answer this question.

Hot Network Questions

C-string appender for buffers and strings

4 Answers 4

You must log in to answer this question.

Related

Hot Network Questions