Reading stream char by char

Question

I'm using this function to read, char by char, a text file or a stdin input

void readLine(FILE *stream, char **string) {
    char c;
    int counter = 0;

    do {
        c = fgetc(stream);
        string[0] = (char *) realloc (string[0], (counter+1) * sizeof(char));
        string[0][counter++] = c;
    } while(c != ENTER && !feof(stream));

    string[counter-1] = '\0';
}

But when I call it, my program crashed and I really don't know why, because I don't forget the 0-terminator and I'm convinced that I stored correctly the char sequence. I've verified the string length, but it appears alright.

[Please don't cast the return value of `malloc()` and friends, in C](http://stackoverflow.com/a/605858/28169). — unwind, Aug 30 '13 at 08:23
@nishant This clearly needs pointer to pointer since it's changing the caller's pointer. What's odd is the choice of `[]` access, it would normally be written `*string = realloc(...)`. — unwind, Aug 30 '13 at 08:24
@unwind: I am sorry, I got confused, In that case, `*string` will be better way to represent it, instead of `string[0]`. Although its correct. — 0xF1, Aug 30 '13 at 08:30

CodeClown42 · Answer 1 · 2013-08-30T09:09:04.097

2

This is an error:

do {
    c = fgetc(stream);
    // What happens here?!? 
} while(c != ENTER && !feof(stream));

"What happens here" is that you add c to string before you've checked for EOF, whoops.

This is very ungood:

    string[0] = (char *) realloc (string[0], (counter+1) * sizeof(char));

in a loop. realloc is a potentially expensive call and you do it for every byte of input! It is also a silly and confusing interface to ask for a pointer parameter that has (apparently) not been allocated anything -- passing in the pointer usually indicates that is already done. What if string were a static array? Instead, allocate in chunks and return a pointer:

char *readLine (FILE *stream) {
// A whole 4 kB!
    int chunksz = 4096;
    int counter = 0;
    char *buffer = malloc(chunksz);
    char *test;
    int c;
    if (!buffer) return NULL;

    while (c = fgetc(stream) && c != ENTER && c != EOF) {
        buffer[counter++] = (char)c;
        if (counter == chunksz) {
            chunksz *= 2;
            test = realloc(buffer, chunksz);
        // Abort on out-of-memory.
            if (!test) {
                free(buffer);
                return NULL;
            } else buffer = test;
        }
    }
// Now null terminate and resize.
    buffer[counter] = '\0';
    realloc(buffer, counter + 1);
    return buffer;
}

That is a standard "power of 2" allocation scheme (it doubles). If you really want to submit a pointer, pre-allocate it and also submit a "max length" parameter:

void *readLine (FILE *stream, char *buffer, int max) {
    int counter = 0;
    int c;

    while (
        c = fgetc(stream)
        && c != ENTER
        && c != EOF
        && counter < max - 1
    ) buffer[counter++] = (char)c;
// Now null terminate.
    buffer[counter] = '\0';
}

edited Aug 30 '13 at 09:09

answered Aug 30 '13 at 09:03

CodeClown42

11,194
1
32
67

Your second tip look better in this case.. But I don't want to use a max length parameter, because I don't know what's the size of my stream input.. – GSchimiti Aug 31 '13 at 02:45
And the garbage stored (\n or EOF) is replaced by 0-terminator after, but I don't know if it's nice.. – GSchimiti Aug 31 '13 at 03:40
I'm pretty late to the party, but when you `realloc()` buffer at the end of first example, is it to save space? Should I actually write `buffer = realloc(buffer, counter + 1);` instead because `realloc()` returns a pointer? Thanks in advance! – Nicholas Humphrey Aug 25 '19 at 15:43
1

Yeah, that trims the space to the actual used size. It's probably not worthwhile unless `chunksz` has gotten relatively large or you are going to use this to store a lot of separate strings. – CodeClown42 Aug 25 '19 at 15:48
Thanks! BTW I think `while (c = fgetc(stream) && c != ENTER && c != EOF)` should be `while ((c = fgetc(stream)) && c != ENTER && c != EOF)` ? – Nicholas Humphrey Aug 25 '19 at 20:16
It doesn't make any difference there, since either way the first clause evaluates to the value of `fgetc(stream)` and is assigned to `c` -- which is a bit of a flaw if `stream` isn't text. Where it would make a difference is if we want to be more robust (allowing for null chars) and succinct: `while (c = fgetc(stream) != EOF && c != ENTER)`. That assigns `fgetc(stream) != EOF` to `c`, meaning `c` will always be either 1 or 0. But `while ((c = fgetc(stream)) != EOF && c != ENTER)` preserves the value of `c` and avoids the null char pitfall. See what 6 more years of S.O. has done for me :D – CodeClown42 Aug 25 '19 at 20:43

score 1 · Answer 2 · answered Aug 30 '13 at 08:19

1

Change this line

string[counter-1] = '\0';

to

string[0][counter-1] = '\0';

You want to terminate string stored at string[0].

answered Aug 30 '13 at 08:19

Rohan

52,392
12
90
87

score 1 · Accepted Answer · edited May 23 '17 at 10:25

1

There are a few issues in this code:

fgetc() returns int.
Don't cast the return value of malloc() and friends, in C.
Avoid using sizeof (char), it's just a very clumsy way of writing 1, so multiplication by it is very redundant.
Normally, buffers are grown more than 1 char at a time, realloc() can be expensive.
string[0] would be more clearly written as *string, since it's not an array but just a pointer to a pointer.
Your logic around end of file means it will store the truncated version of EOF, not very nice.

edited May 23 '17 at 10:25

Community

1
1

answered Aug 30 '13 at 08:26

unwind

391,730
64
469
606

@nishant : #6 is consequent of #1. – CodeClown42 Aug 30 '13 at 09:03
Ok, got it. You mean to say `EOF` is stored as `char` then stored back at `string`. – 0xF1 Aug 30 '13 at 09:42

Reading stream char by char

3 Answers3