Re: strfile: new module

[Simon Josefsson]

Bruno Haible <address@hidden> writes:

>> Index: modules/strfile
>
> I would call the module 'read-file' and the function 'read_file'. So that it's
> possible to add a module 'write_file' if needed, and for consistency with the
> 'copy-file' module.

I like this, and I also like your fread_file function.  I will create
a module read-file with two functions fread_file and read_file.

>> +  do {
>
> In GNU style the brace goes on the next line.

I'll indent it.

Hmmm.  I've made the same mistake before.  I'll see how hard it would
be to write a tool that will "prepare" additions of a gnulib module,
for one it should run indent on the files, and then run 'cvs
--new-file diff' on the modules file, and the files mentioned in the
modules file.  Eventually, it could apply the tests from the
maintainer-makefile module.

>> +    tmp = realloc (out, pos + BUFSIZ);
>
> Quadratic runtime behaviour: if you don't have particular luck with the 
> realloc()
> implementation, for large files, this loop will spend most of its time in
> realloc(), copying memory around.

Your fread_file may solve this, and read_file will use it.

>> +  if (!(feof (fh) && fclose (fh)))
>
> Missing treatment of the ferror (fh) case.

Same here.

> As additional input, find attached some similar (unpolished) code I wrote 
> long ago
> in the past. The good point about a function that takes a FILE stream rather
> than a filename as argument is that the caller has more liberty to decide
> about O_TEXT / O_BINARY and about what to do when the file does not exist.

Agreed.

> The downside is, of course, that it's not so immediate to use this
> function. But maybe the module can provide both: fread_file that
> reads from a stream, and read_file that takes a filename?

Exactly!

/Simon