A full exegesis on this code will take a while.
The brief version is:
FILE *openCatalog(char catalogname[]) { FILE *fp = fopen(catalogname, "r"); return(fp); }
This opens the named file for reading and returns the file stream pointer.
An alternative brief version checks whether the file can be opened:
int openCatalog(char catalogname[]) { FILE *fp = fopen(catalogname, "r"); if (fp != 0) { fclose(fp); return 1; } else { return 0; } }
Exegesis
The original code is:
FILE* openCatalog(char catalogname[]) { catalogname = fopen ("*catalogname", "r"); if(fopen != 0) { return 1; } else { return 0; } }
We don't have a specification beyond 'open the file'. Given the function signature, it appears that you want the FILE * (file stream pointer) for the open file returned. So, let's critique the code on that basis.
The function prototype line is OK; it could be char const catalogname[] to emphasize that the function won't modify the file name, but that's a refinement, not a bug-fixing change.
The type of catalogname in the function is char *. In a function's argument list, an array is loosely equivalent to a pointer. I'd normally write FILE *openCatalog(char const *catalogname) to emphasize that within the function, it is a char const * variable. Nevertheless, it is 100% legitimate to use the array notation you used; it is purely a stylistic preference for me to use the pointer notation.
The next executable line has a couple of problems. The function call to fopen() is not technically wrong, though it opens a file with a fixed name *catalogname rather than the file specified in the variable catalogname. To fix that, you need to remove the quotes. The * would give you a character instead of a string, and it would be the first character of the file name. So, remove the * too.
This leaves you with fopen() returning a value, actually a FILE *, and you assign it to catalogname, a char *. This is a type mismatch, which the compiler warns about. As shown in the first rewrite, the more normal way to handle this would be:
FILE *fp = fopen(catalogname, "r");
This accounts for your error message:
a3.c:20: warning: assignment from incompatible pointer type
We do not know whether your catalog is a text file or a binary file. If it is a binary file, then you should use "rb" to open it if you are on Windows (where it really matters), and that will work fine on Unix-like systems too (where there is no distinction between text and binary files).
The next line in your code is the conditional:
if (fopen != 0)
This actually checks whether the function pointer for the function fopen() is null or not. And the C standard guarantees that in a hosted environment (which you are using), no function pointer will be null. So the compiler can optimize that test to assume that the condition is always true.
What you actually need is a test of the result:
if (fp != 0)
You then have two return statements, one returning 1, one returning 0. The one returning 1 elicits a warning about converting an integer to a pointer because the function is defined to return a FILE * and 1 is an integer.
This accounts for your other warning message:
a3.c:22: warning: return makes pointer from integer without a cast
The return returning 0 does not generate a warning because 0 is a null pointer constant, and that is a valid value for a function like this to return.
The code should probably simply return the value from fopen(). It is correct to test the value somewhere, either in this function or in the calling function, to ensure that the open succeeded. You might test it here with:
if (fp == 0) err_report_and_exit("Failed to open file %s (%d: %s)\n", catalogname, errno, strerror(errno));
Using errno and strerror() means that you should also include the headers:
#include <errno.h> #include <string.h>
The code should return the file stream from fopen():
return fp;
In total, that leads to:
FILE *openCatalog(char catalogname[]) { FILE *fp = fopen(catalogname, "r"); if (fp == 0) err_report_and_exit("Failed to open file %s (%d: %s)\n", catalogname, errno, strerror(errno)); return(fp); }
The problems cited above are much the same if the function is intended to check whether the file can be opened for reading. The revised code I showed first is slightly verbose. Since the presumed purpose is to check whether the file can be opened, the calling code should be responsible for dealing with the 'it cannot be opened case'; it knows what sort of diagnostic to generate. This is a slightly more compact version of the revision, but the object code for the this and the one above would be identical.
int openCatalog(char catalogname[]) { FILE *fp = fopen(catalogname, "r"); if (fp != 0) { fclose(fp); return 1; } return 0; }
A simple implementation of err_report_and_exit() is:
#include "errreport.h" #include <stdio.h> #include <stdlib.h> #include <starg.h> void err_report_and_exit(char const *format, ...) { va_list args; va_start(args, format); vfprintf(stderr, format, args); va_end(args); exit(EXIT_FAILURE); }
The "errreport.h" header might be:
#ifndef ERRREPORT_H_INCLUDED #define ERRREPORT_H_INCLUDED #ifdef __GNUC__ #define PRINTFLIKE(n,m) __attribute__((format(printf,n,m))) #define NORETURN() __attribute__((noreturn)) #else #define PRINTFLIKE(n,m) /* If only */ #define NORETURN() /* If only */ #endif /* __GNUC__ */ extern void err_report_and_exit(char const *format, ...) PRINTFLIKE(1,2) NORETURN(); #endif /* ERRREPORT_H_INCLUDED */
The GCC-specific bits mean that you get the same level of format error reporting as if you were using printf() directly.
(This code is modelled on a bigger package that systematically uses err_ as a function prefix. In that package, this function would be err_error(). That's why it is err_ rather than error_ — though the explanation takes longer to type than it would to fix it. The big package is contained in files stderr.c and stderr.h, though there's an argument that it is treading on thin ice using the std prefix. It is, however, my standard error reporting package.)
