2
\$\begingroup\$

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

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

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.

\$\endgroup\$
4
\$\begingroup\$
  • 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.

\$\endgroup\$
3
\$\begingroup\$

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.

\$\endgroup\$
0
\$\begingroup\$

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?

\$\endgroup\$

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.