Paul Eggert <[EMAIL PROTECTED]> writes: > Looks nice. A couple of comments: > >> + count = fread (buf + size, 1, BUFSIZ, stream); > > BUFSIZ is too small on many systems, for backward compatibility reasons. > Why not change that "BUFSIZ" to "alloc - size - 1"?
Done. > Also, for the common case of regular files that you have opened, > you can use fstat to cheaply find out an estimated size for the file, > and use that estimate rather than guessing and growing the buffer. > The estimate might be wrong (due to binary file translation, or > growing sizes), but still, it's better than a guess. > > Even if it is a regular file that you have not opened, you can still > estimate the size of the remaining part of the file by subtracting > lseek (..., SEEK_CUR) from fstat. Yup. It's slightly less portable, perhaps, but if someone writes the code (especially the M4 test), adding this seems like a good idea. It will probably be easier to write the code once the current module has been installed though. The mmap approach is even less portable, and also require a call to munmap, so I'm less sure about it. I'm just looking for something very simple to read X.509 certificates, Kerberos tickets etc, which are typically only a few kb's large. I'll start to use the module in GnuTLS and Shishi to see how it works now. >> + if (buf) >> + free (buf); > > How about just 'free (buf);'; that's smaller. I prefer that to, but Ralf said to depend on the "free" module to make sure free (NULL) works. The free module is GPL, and I need this module to be LGPL, so I can't use it, hence handling this in the code. I think we should be able to assume free (NULL) works though, even my K&R says it should work, and without depending on the free module. /Simon
