How to dynamically expand a string in C

Question

I have a function that recursively makes some calculations on a set of numbers. I want to also pretty-print the calculation in each recursion call by passing the string from the previous calculation and concatenating it with the current operation. A sample output might look like this:

3
(3) + 2
((3) + 2) / 4
(((3) + 2) / 4) x 5
((((3) + 2) / 4) x 5) + 14
... and so on

So basically, the second call gets 3 and appends + 2 to it, the third call gets passed (3) + 2 , etc. My recursive function prototype looks like this:

void calc_rec(int input[], int length, char * previous_string);

I wrote a 2 helper functions to help me with the operation, but they implode when I test them:

/**********************************************************************
 * dynamically allocate and append new string to old string and return a pointer to it
 **********************************************************************/
 char * strapp(char * old, char * new)
 {
     // find the size of the string to allocate
     int len = sizeof(char) * (strlen(old) + strlen(new));

     // allocate a pointer to the new string
     char * out = (char*)malloc(len);

     // concat both strings and return
     sprintf(out, "%s%s", old, new);

     return out;
 }

/**********************************************************************
 * returns a pretty math representation of the calculation op
 **********************************************************************/
 char * mathop(char * old, char operand, int num)
 {
     char * output, *newout;
     char fstr[50]; // random guess.. couldn't think of a better way.
     sprintf(fstr, " %c %d", operand, num);
     output = strapp(old, fstr);
     newout = (char*)malloc( 2*sizeof(char)+sizeof(output) );
     sprintf(newout, "(%s)", output);
     free(output);
     return newout;  
 }


void test_mathop()
{
    int i, total = 10;
    char * first = "3";
    printf("in test_mathop\n");
    while (i < total)
    {
        first = mathop(first, "+", i);
        printf("%s\n", first);
        ++i;
    }
}

strapp() returns a pointer to newly appended strings (works), and mathop() is supposed to take the old calculation string ("(3)+2"), a char operand ('+', '-', etc) and an int, and return a pointer to the new string, for example "((3)+2)/3". Any idea where I'm messing things up? thanks.

Answer 1

You should allocate an extra byte for the terminating 0.

And you should use strlen instead of sizeof here:

newout = (char*)malloc( 2*sizeof(char)+sizeof(output) );

Sizeof returns the size of char* (which is something like 4 on an average system) instead of the length of the string pointed to by output.

Moreover, as @Agnel rightly pointed out, i is uninitialized, although that does not make your program crash, just execute the loop for a random number of times.

Also, I would recommend strstr strcat for concatenating your strings.

Answer 2

One immediate problem I can see is:

int len = sizeof(char) * (strlen(old) + strlen(new));

does not allocate space for the NULL char at the end. So you need one additional char allocated.

Answer 3

Try this, for starters:

char * append_strings(const char * old, const char * new)
{
    // find the size of the string to allocate
    size_t len = strlen(old) + strlen(new) + 1;

    // allocate a pointer to the new string
    char *out = malloc(len);

    // concat both strings and return
    sprintf(out, "%s%s", old, new);

    return out;
}

This is just your code, with a number of fixes:

• The input strings are not changed, so they should be declared const.
• Add one to the size required for the new buffer, to have room for the terminator.
• String lengths are best stored in variables of type size_t.
• Removed needless scaling by sizeof (char).
• Don't cast the return of malloc().
• Don't define functions whose name start with str, that is a reserved space. Pointed out by commenter, thanks!

For performance you could make use of the fact that you're calling strlen() on both inputs, and avoid using sprintf():

char * append_strings(const char * old, const char * new)
{
    // find the size of the string to allocate
    const size_t old_len = strlen(old), new_len = strlen(new);
    const size_t out_len = old_len + new_len + 1;

    // allocate a pointer to the new string
    char *out = malloc(out_len);

    // concat both strings and return
    memcpy(out, old, old_len);
    memcpy(out + old_len, new, new_len + 1);

    return out;
}

This should be a bit quicker, but I haven't benchmarked it. Notice how the final memcpy() includes the terminator from the new string, so there's no need to manually set it.