Dynamically created C string

Question

I'm trying to get an expression from the user and put it in a dynamically created string. Here's the code:

char *get_exp() {
    char *exp, *tmp = NULL;
    size_t size = 0;
    char c;
    scanf("%c", &c);

    while (c != EOF && c != '\n') {
        tmp = realloc(exp, ++size * sizeof char);
        if (tmp == NULL)
            return NULL;

        exp = tmp;
        exp[size-1] = c;
        scanf("%c", &c);
    }
    tmp = realloc(exp, size+1 * sizeof char);
    size++;
    exp = tmp;
    exp[size] = '\0';
    return exp;
}

However, the first character read is a newline char every time for some reason, so the while loop exits. I'm using XCode, may that be the cause of the problem?

Checked on ubuntu/gcc - no problem if exp is initialized to NULL (or it will be a crash). Did you tried a simpliest test - just main() + call to get_exp()? And how do you call it, in command line on in XCode? — Michael, Nov 17 '13 at 15:33
Unlike `c = getc()`, `scanf("%c", &c)` can never produce a `EOF` value. So testing for `EOF` is unnecessary. — glglgl, Nov 17 '13 at 15:38
Note that standard IO functions are line buffered, and will not return anything until you press enter. There's not standard way around this, it's OS dependent, and I don't know how to do it in MacOS. Another thing, in a case like this, you can avoid duplicate code by `for(;;) { something...; if (endcondition) break; somethingmore...; }`. — hyde, Nov 17 '13 at 15:44

Jonathan Leffler · Answer 1 · 2013-11-17T15:44:04.280

No, XCode is not part of your problem (it is a poor workman who blames his tools).

You've not initialized exp, which is going to cause problems.

Your code to detect EOF is completely broken; you must test the return value of scanf() to detect EOF. You'd do better using getchar() with int c:

int c;

while ((c = getchar()) != EOF && c != '\n')
{
    ...
}

If you feel you must use scanf(), then you need to test each call to scanf():

char c;

while (scanf("%c", &c) == 1 && c != EOF)
{
    ...
}

You do check the result of realloc() in the loop; that's good. You don't check the result of realloc() after the loop (and you aren't shrinking your allocation); please check every time.

You should consider using a mechanism that allocates many bytes at a time, rather than one realloc() per character read; that is expensive.

Of course, if the goal is simply to read a line, then it would be simplest to use POSIX getline(), which handles all the allocation for you. Alternatively, you can use fgets() to read the line. You might use a fixed buffer to collect the data, and then copy that to an appropriately sized dynamically allocated buffer. You would also allow for the possibility that the line is very long, so you'd check that you'd actually got the newline.

condorwasabi · Answer 2 · 2013-11-17T16:05:10.363

0

Here on Windows XP/cc, like Michael said, it works if exp is initialized to NULL.

edited Nov 17 '13 at 16:05

answered Nov 17 '13 at 15:45

condorwasabi

616
6
19

hyde · Answer 3 · 2013-11-17T16:11:07.563

Here's a fixed code, with comments explaining what is different from your code in the question:

char *get_exp()
{
    // keep variables with narrowest scope possible
    char *exp = NULL;
    size_t size = 0;

    // use a "forever" loop with break in the middle, to avoid code duplication
    for(;;) {

        // removed sizeof char, because that is defined to be 1 in C standard
        char *tmp = realloc(exp, ++size);
        if (tmp == NULL) {
            // in your code, you did not free already reserved memory here
            free(exp); // free(NULL) is allowed (does nothing)
            return NULL;
        }
        exp = tmp;

        // Using getchar instead of scanf to get EOF,
        // type int required to have both all byte values, and EOF value.
        // If you do use scanf, you should also check it's return value (read doc).
        int ch = getchar();
        if (ch == EOF) break; // eof (or error, use feof(stdin)/ferror(stdin) to check)
        if (ch == '\n') break; // end of line
        exp[size - 1] = ch; // implicit cast to char
    }

    if (exp) {
        // If we got here, for loop above did break after reallocing buffer,
        // but before storing anything to the new byte.
        // Your code put the terminating '\0' to 1 byte beyond end of allocation.
        exp[size-1] = '\0';
    }
    // else exp = strdup(""); // uncomment if you want to return empty string for empty line
    return exp;
}

As said by others, note that growing the buffer one byte by one byte is very inefficient. You should double the buffer with each realloc, then finally realloc it to correct size when entire line is read and exact size is known. — hyde, Nov 17 '13 at 16:13

Dynamically created C string

3 Answers3