Simon Josefsson wrote:
> Here's the module. Only lighted tested, mostly to get feedback on the
> approach, but if you agree with it, detailed review would be useful.
Looks already quite good. Only minor nits:
> +/* Read a STREAM and return a newly allocated string with the content,
> + and set LENGTH to the length of the string.
It sets *LENGTH, not LENGTH, I think.
> The string is
> + zero-terminated, but the terminating zero character is not counted
I'd prefer to write "zero byte" instead of "zero character", because a
character nowadays can generally be multibyte.
> + in the LENGTH variable. On errors, return NULL, sets errno and
On systems like mingw (which don't attempt POSIX compliance) errno is
not set when fread() fails.
> + LENGTH is undefined. */
*LENGTH here as well.
> + char *tmp;
I would rename this variable to 'new_buf'.
> + buf[size] = '\0';
This crashes if the stream was initially already at EOF. In this case
you must explicitly malloc at least 1 byte.
> +static char *
> +internal_read_file (const char *filename, size_t * length, const char *mode)
> +{
> + FILE *stream = fopen (filename, mode);
> + char *out = NULL;
The NULL initializer is unnecessary.
> +/* Open and read the contents of FILENAME, and return a newly
> + allocated string with the content, and set LENGTH to the length of
> + the string. The string is zero-terminated, but the terminating
> + zero character is not counted in the LENGTH variable. On errors,
> + return NULL and sets errno. */
Same comments as above regarding *LENGTH and errno.
> +char *
> +read_file (const char *filename, size_t * length)
> +char *
> +read_binary_file (const char *filename, size_t * length)
These functions are so similar, that I would merge them into one:
char *
read_file (const char *filename, size_t *length, bool textmode)
> +Depends-on:
> +realloc
You don't need this dependency, since the 2nd argument you pass to realloc()
is always positive.
Bruno
