I'm new to C and getting confused with the pointers and char arrays...

I have this written:

char *read(char *filename) {
    char *text[1000];
    FILE *inputFile = fopen(filename, "r");
    int i=0;
    while (feof(inputFile)) {
        text[i  ] = fgetc(inputFile);
    }
    text[i]='\0';
    fclose(inputFile);
    return text;
}

My goal is to pass in the name of the file that I want to open, open it, and assign all of the words in it into a char array (char text[]). I continually get errors regarding:

"expression which evaluates to zero treated as a null pointer constant of type 'char *' [-Wnon-literal-null-conversion]"

"incompatible pointer types returning 'char *[1000]' from a function with result type 'char *' [-Wincompatible-pointer-types]"

Looking for any advice.

CodePudding user response：

You have two issues here.

First your array is declared as char *text[1000]; i.e. an array of pointers to char, so each text[i] is a pointer, not a character. What you probably wanted is char text[1000];.

This then leads to the second problem, which is that you're returning a pointer to a local variable. That pointer becomes invalid when the function returns, so attempting to use it will trigger undefined behavior.

You should instead allocate the memory dynamically:

char *text = malloc(1000);

So it will still be valid when the function returns. Keep in mind that you'll need to free the memory when you're done using it.

Alternately, you can pass in the buffer to fill as an argument to the function:

void read(char *filename, char *text) {

CodePudding user response：

You want to fill a character array with characters read from a file. If so then at least you need to declare an array of characters like

char text[1000];

instead of an array of pointers to characters as you wrote

char *text[1000];

However the declared array has automatic storage duration and will not be alive after exiting the function. So the returned pointer will be invalid.

You should to allocate an array dynamically as for example

char *text = malloc( 1000 );

Also you need to check whether the file was opened successfuly.

The condition in the while loop

while (feof(inputFile)) {
    text[i  ] = fgetc(inputFile);
}

can occur after the call of fgets. As a result the array can store an invalid character.

You should write

for ( int value; i   1 < 1000 && ( value = fgetc( inputFile ) ) != EOF; i   ) 
{
    text[i] = value;
}

CodePudding user response：

The crux of the problem is: Where is that memory space into which you store those characters located? And - who can use after your function returns? Specifically, what happens when your function is called again?

Right now (and ignoring the fact you have an array-of-pointers), you're returning the address of a local variable, as @dbush points out. That means that:

• Only your function can use that space in memory.
• It stops being usable when you leave the function...
• ... and the same addresses may be allocated for other uses in your program, by the compiler.

@dbush' suggestion is one way to go about fixing that: Your function can allocate memory on every call, and hand back a pointer to that allocated memory.

Another alternative is for the function's signature to change, so that it's the caller who provides the memory space, e.g. :

int read_entire_file(char* destination_buffer, const char *filename);

in both cases, there's another problem, which is the 1000 character limit (or rather, 999 characters and the trailing '\0' which marks the end of a string). What if there's more data in the file? Perhaps you should actually take the buffer's size as well?:

int read_entire_file(char* destination_buffer, size_t buffer_size, const char *filename);

There are other alternatives to the one I suggested and @dbush 's suggestion, but my point is that there's more than one way to approach this task.

Other comments/issues:

• Don't take the filename as a non-cost pointer (char*); make it const char* filename to clarify you're not allowed to change the filename.
• fgetc() is a rather slow way to read data from a file. Consider fread():

  size_t fread(void *ptr, size_t size, size_t nmemb, FILE *stream);
  

• You must ensure your fopen() file succeeded; and if it hasn't, return NULL (or terminate the program). Same goes for your fgetc() call.
• You're not checking whether you've reached the edge of your buffer, and may thus overflow it.
• It's not a great idea to place large amounts of data on the stack.
• read() is an overly generic name; be more specific. Also, the name might clash with a typical system call of the same name on Linux, MacOS and other Unix-like operating systems.