bug-gnulib
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: strfile: new module


From: Simon Josefsson
Subject: Re: strfile: new module
Date: Thu, 01 Jun 2006 16:37:46 +0200
User-agent: Gnus/5.110006 (No Gnus v0.6) Emacs/22.0.50 (gnu/linux)

Bruno Haible <address@hidden> 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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]