C easy code with filling array

Question

I'm trying to fill an array with a file called data.txt. I don't know what's wrong with code. I get segmentation fault: 11 error.

#include <stdio.h>
#include <stdlib.h>

void input(int arr[]){
    FILE* f;
    int x, i=0;
    f= fopen("data.txt","r");
    while (arr[i] != EOF){
        fscanf(f,"%d",&x);
        arr[i] = x;
        i++;
    }
    fclose(f);
}

int main(){
    int arr[50];
    input(&arr[50]);
    printf("%d", arr[0]);
}

`arr[i]` will never equal `EOF`, so you loop forever and eventually segfault. You also pass the address of the first element past the end of your array to `input()`, so you're trying to write into memory you don't own from the outset. — Crowman, Apr 07 '17 at 03:36
You don't check that you successfully opened the input file before using the file pointer. That is an easy cause of crashes (and easily fixed — always check the returned value from `fopen()` or any other open-like function). You don't check the return value from `fscanf()`; that too is a mistake. (See [How do we check the return values from `scanf()`?](http://stackoverflow.com/questions/10084224)) — Jonathan Leffler, Apr 07 '17 at 04:16
the statement: `while (arr[i] != EOF)` on each iteration through the loop is looking at an entry in `arr[]` that is not yet initialized! Suggest replacing the `while()` and the next statement with: `while( 1 == fscanf(f,"%d",&x) )` also the loop has not limit on the number of integers that can be read, but the actual array is declared with only 50 entries, so the while() statement should be: `while( i<50 && 1 == fscanf(f,"%d",&x) )` — user3629249, Apr 07 '17 at 21:06
in C, an array offset/index starts at 0 and continues to (length of array -1) and this statement: `input(arr[50]);` is passing the value of the (one past the end of the array) to the function `input()`. The line should be: `input(arr);` — user3629249, Apr 07 '17 at 21:08
the posted code contains some 'magic' numbers. 'magic' numbers are numbers with no basis. I.E. 50 'magic' numbers make the code much more difficult to understand, debug, etc. Suggest using a `enum` statement or `#define` statements to give those magic numbers meaningful names, then use those meaningful names throughout the code. — user3629249, Apr 07 '17 at 21:10

score 2 · Answer 1 · edited Apr 07 '17 at 04:13

2

You are reading the number into x (which you are copying into arr[i]) and then comparing arr[i+1] to EOF. That is not how it has to be done.

Try this

while (fscanf(f, "%d", &arr[i]) == 1) 
    i++;

But this would violate so many safety constraints. Better also bound check and break early if i is greater than some limit, but that limit should be passed to the function.

Another error is with how you are passing arguments to input. Pass input(arr) instead of input(&arr[50]). If you want to use & use input(&arr[0]).

edited Apr 07 '17 at 04:13

Jonathan Leffler

730,956
141
904
1,278

answered Apr 07 '17 at 03:41

Ajay Brahmakshatriya

8,993
3
26
49

Oh — surely not! (Apart from missing `f` as the first argument to `fscanf()`…) Why not `while (fscanf(f, "%d", &arr[i]) == 1) i++;`, for example? Infinite loops are bad in general. Sometimes necessary, but this isn't one of those times. Yes, you terminate if `fscanf()` returns 0 at some time, but you keep spinning indefinitely if it returns `EOF`. – Jonathan Leffler Apr 07 '17 at 03:59
@JonathanLeffler I hope this is better? – Ajay Brahmakshatriya Apr 07 '17 at 04:03
Well, I suppose so. I still prefer `while (fscanf(f, "%d", &arr[i]) == 1) i++;` as the loop. I'm skipping array boundary checking because the size isn't passed to the function. There are advantages to this. One is that `i` is not incremented when the conversion fails, so you don't have to worry about whether or not to decrement `i` after the loop to report how many entries were created. It also doesn't need a separate status variable. – Jonathan Leffler Apr 07 '17 at 04:06
Again, I wrote separate conditions for 0 and EOF instead of 1 so that it is more clear what we are checking. Since OP wrote EOF in his question. – Ajay Brahmakshatriya Apr 07 '17 at 04:07
OK; opinions (yours and mine) differ. I'm testing that the number of values converted matches the number of conversion specifications listed in the format string. You're checking two statuses. I don't need a variable to hold the status (though if I want to check, it is easy to add one: `int n_cvt; while ((n_cvt = fscanf(f, "%d", &arr[i])) == 1) i++;`). Note that if there were 7 values being scanned, all I'd change is the format string, the list of variables, and the value 1 to 7. You'd have to consider what to do about return values EOF, 0, 1, 2, 3, 4, 5, 6. That's hard work! – Jonathan Leffler Apr 07 '17 at 04:11
@JonathanLeffler okay so.... what am I suppose to do with this lol... you guys are confusing me more. – were.erer Apr 07 '17 at 04:34
@were.erer We arrived at a consensus. The one that is in the answer right now. Though both of us with suggest - add some bound check on how many you read. You don't want to overflow. – Ajay Brahmakshatriya Apr 07 '17 at 04:35
@were.erer: You can use Ajay's code with some caveats that he's mentioned — you don't know how big the array is in the function, so you can't properly limit the input; and your `main()` function doesn't know how many entries were read. Or you can look at the code in my answer. – Jonathan Leffler Apr 07 '17 at 05:07

Jonathan Leffler · Answer 2 · 2017-04-07T05:03:45.240

This would be more nearly my version of your code:

#include <stdio.h>
#include <stdlib.h>

static int input(int size, int arr[])
{
    const char file[] = "data.txt";
    FILE *f = fopen(file, "r");
    if (f == NULL)
    {
        fprintf(stderr, "Failed to open file '%s' for reading\n", file);
        exit(EXIT_FAILURE);
    }

    int i;
    for (i = 0; i < size && fscanf(f, "%d", &arr[i]) == 1; i++)
        ;

    fclose(f);
    return i;
}

int main(void)
{
    int arr[50];
    int num = input(50, arr);
    for (int i = 0; i < num; i++)
        printf("%d: %d\n", i, arr[i]);
    return 0;
}

The use of static before the function is necessary to quell -Wmissing-prototypes. The main() function tells the input() function how many elements are in the array so the input() function can avoid overflowing the buffer (a stack overflow, no less). The input() function tells the main() function how many values it read, so the main() function doesn't go accessing data that was not initialized. The crucial function calls are error checked — fopen() and fscanf().

The code compiles cleanly on a Mac running macOS Sierra 10.12.4 using GCC 6.3.0 and the command line below (the source file was rf19.c):

$ gcc -O3 -g -std=c11 -Wall -Wextra -Werror -Wmissing-prototypes \
>     -Wstrict-prototypes -Wold-style-definition rf19.c -o rf19
$

I generated a data file with 23 random integers between 10 and 99 in it, and the output was:

That's not dreadfully informative, but better than nothing.

The code has deficiencies still, which I don't plan to fix — some more serious than others. For example, the file name is fixed — that's a no-no. The code in the input() function exits on error; that isn't necessarily OK. It produces an error message on standard error — that's better than standard output, but wouldn't be a good idea in a GUI application. The output wastes a lot of horizontal space; with the data shown, you could get 10 values per output line (that would use about 70 characters per line), but the printing for that is more intricate, so I didn't show it. The code treats EOF and a word or punctuation character in the data the same; that might or might not matter, depending on your application. The input simply stops after the 50th entry; maybe you need to know whether there were more entries available to read. I'd probably process the command line arguments as file names, or process standard input if no files were specified — the Unix 'filter command' idiom. I'd probably do something more exciting than just print the first fifty values. I'd probably put the file reading code in a separate function from the file open/close code.

C easy code with filling array

2 Answers2