1

I've been trying to piece together a function that allows me to create a string out of a given file of unknown length.

What it's supposed to do, is set the size of the output string to a single character, then for each character besides EOF, increment the size of the string by 1 and add the newly read character to it.

void readstring(FILE *f, char *s[])
{
    int size = 1;
    int c = 0, i = 0;

    s = malloc(size*sizeof(char));

    while(c != -1)
    {
        c = fgetc(f);
        s[i] = (char)c;
        i++;

        if(i == size)
        {
            size++;
            s = realloc(s, size*sizeof(char));
        }
    }

    s[i] = '\0';
}

int main()
{
    char *in = malloc(2*sizeof(char));
    FILE *IN;

    IN = fopen("in.txt", "r");

    readstring(IN, in);

    printf("%s",&in);

    fclose(IN);
    free(in);

    return 0;
}
  • 2
    **1st** `EOF` is not a character – pmg Jan 16 '20 at 11:21
  • After `char in[2];` you simply cannot change the size of `in` in any way, not even hiding it inside a function. Try `char *in = malloc(2);` and remember to `free(in);` before your program terminates. – pmg Jan 16 '20 at 11:23
  • 1
    You also need to make sure you allocate 1 more character than you need to store that `\0` at the end of your function – Chris Turner Jan 16 '20 at 11:33
  • https://stackoverflow.com/questions/605845/do-i-cast-the-result-of-malloc – RobertS supports Monica Cellio Jan 16 '20 at 11:35
  • Good news is, after making the changes you recommended, I no longer get any errors or warnings. Bad news is, it still seems to spew out a single random character instead of the content of the IN file. I'm assuming it's due to the ```realloc()``` part of the function, but I still haven't figured out what exactly goes wrong. – PossiblyIncompetent Jan 16 '20 at 11:40
  • You should update your code in the question to reflect the changes you have made based on the comments. – Andrew Truckle Jan 16 '20 at 11:45
  • Does he have to pass a reference to a pointer as the parameter? Else he is passing a copy. Might that cause the result to be different in main? – Andrew Truckle Jan 16 '20 at 11:46
  • A question is a sentence with a question mark at the end. Unfortunately, I was not able to find a question mark in your post... – cmaster - reinstate monica Jan 16 '20 at 11:52
  • 1
    This seems holy inefficient, allocating one char at a time. Consider allocating in chunks of e.g. 128 bytes. – Paul Ogilvie Jan 16 '20 at 11:52
  • Since `realloc` can change the pointer, you need to pas a double pointer. – Paul Ogilvie Jan 16 '20 at 11:54
  • Wouldn't allocating more chunks at a time result in some empty spaces in the end? Not saying that would be difficult to deal with, I assume I'd just do something along the lines of ```realloc(strlen(s)*sizeof(char))```, but I thought adding them one character at a time is more efficient. – PossiblyIncompetent Jan 16 '20 at 11:56
  • Memory reallocation is expensive. Do it in chunks. It doesn’t matter about redundant buffer since null character signals end of string. – Andrew Truckle Jan 16 '20 at 13:36
  • And in addition to memory reallocation being expensive, it also has quadratic complexity (`O(N^2)`) if you don't grow your increments. Every time that you realloc the string, you are possibly copying over a string of increasing length. The normal approach is to use increments of factor 2, which gets the complexity down to linear (`O(N)`) as every byte is only copied once or twice on average. – cmaster - reinstate monica Jan 16 '20 at 13:49

1 Answers1

1

If you are on a POSIX compliant system (any modern Linux), don't try to reinvent the wheel. Just use getline(). It will do the tricky stuff of managing a growing buffer for you, and return a suitably malloc()'ed string.


You are assigning the result of malloc()/realloc() to s instead of *s. s is purely local to readstring(), *s is the pointer variable that s points to. Pass in the adress of the pointer in main. Correct code looks like this:

void foo(char** out_string) {
    *out_string = malloc(...);
}

int main() {
    char* string;
    foo(&string);
}

Without the address taking & and pointer dereferenciation *, your main() cannot know where readstring() has stored the characters.

You also need to dereference the double pointer when you set the characters of the string: Use (*s)[i] instead of s[i], as s[i] denotes a pointer, not a character.


Also, try running your program with valgrind, and see if you can learn from the errors that it will spew out at you. It's a great tool for debugging memory related problems.

cmaster - reinstate monica
  • 38,891
  • 9
  • 62
  • 106