Bruno Haible <[EMAIL PROTECTED]> writes:
> 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.
Fixed.
>> 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.
Changed.
>> + 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.
I've changed it to just say that it doesn't distort errno:
the LENGTH variable. On errors, *LENGTH is undefined, errno
preserves the system function exit codes (if any), and NULL is
returned. */
>> + LENGTH is undefined. */
>
> *LENGTH here as well.
Yup, done.
>> + char *tmp;
>
> I would rename this variable to 'new_buf'.
Done.
>> + buf[size] = '\0';
>
> This crashes if the stream was initially already at EOF. In this case
> you must explicitly malloc at least 1 byte.
No, the allocated size computed earlier is one more that is read using
fread, so I think it works. I've incorporated Paul's suggestion on
improving the fread call, so please double check this below.
>> +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.
Removed.
>> +/* 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.
Yup, updated.
>> +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)
I prefer to keep this in the function name. Also the "textmode"
concept doesn't seem to be a POSIX concept (at least my fopen man page
says the "b" modified is only for ISO C89 compatibility, and is
irrelevant on all POSIX systems).
>> +Depends-on:
>> +realloc
>
> You don't need this dependency, since the 2nd argument you pass to realloc()
> is always positive.
Yup, removed.
/Simon
