CODEWITHSUNDEEP
carraysstringrealloc
balinthaller
balinthaller

Reputation: 38

Dynamically created C string

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?

Upvotes: 0

Views: 101

Answers (3)

hyde
hyde

Reputation: 62906

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;
}

Upvotes: 0

condorwasabi
condorwasabi

Reputation: 626

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

Upvotes: 0

Jonathan Leffler
Jonathan Leffler

Reputation: 755104

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.

Upvotes: 1

Related Questions