diff: io__read_line_as_string (second round)
Fergus Henderson
fjh at cs.mu.OZ.AU
Thu Feb 12 13:38:53 AEDT 1998
On 12-Feb-1998, Andrew Bromage <bromage at cs.mu.OZ.AU> wrote:
> Add io__read_line_as_string/{3,4}, make io__read_line a bit more
> efficient.
io.m:
> io__read_line(Stream, Result) -->
> +#define READ_LINE_GROW(n) ((n) * 3 / 2)
> +#define BYTES_TO_WORDS(n) (((n) + sizeof(Word)) / sizeof(Word))
That should be
#define BYTES_TO_WORDS(n) (((n) + sizeof(Word) - 1) / sizeof(Word))
^^^
> +#define READ_LINE_START 2048
> +
> + Char initial_read_buffer[READ_LINE_START];
> + Char *read_buffer = initial_read_buffer;
> + size_t read_buf_size = READ_LINE_START;
> + Integer i;
> + Char char_code = '\\0';
Making `i' of type `Integer' will work, but conceptually
`int' or `size_t' would be better, since it's just a counter
and doesn't have to fit in a Mercury register or anything like that.
char_code needs to be of type `int' or `Integer',
so it can handle the < 0 cases.
> + Res = 0;
> + for (i = 0; char_code != '\\n'; ) {
> + char_code = mercury_getc((MercuryFile*)File);
Our standard layout for type casts is
char_code = mercury_getc((MercuryFile *) File);
^ ^
> + if (char_code == -1) {
> + if (i == 0) {
> + Res = -1;
> + }
> + else {
Should be "} else {", not on two lines.
> + Res = 0;
> + }
> + break;
You already assigned `Res = 0;', it seems a shame to do it twice.
> + }
> + if (char_code != (char_code & 0xFF)) {
This would be better written
if (char_code != (Char) char_code)
to avoid the magic number 0xFF.
> + Res = -2;
> + break;
> + }
> + read_buffer[i++] = char_code;
> + MR_assert(i <= read_buf_size);
> + if (i == read_buf_size) {
> + /* Grow the read buffer */
> + read_buf_size = READ_LINE_GROW(read_buf_size);
> + if (read_buffer == initial_read_buffer) {
> + read_buffer = checked_malloc(
> + BYTES_TO_WORDS(read_buf_size
> + * sizeof(Char)));
checked_malloc() allocates in units of bytes, not words.
> + memcpy(read_buffer, initial_read_buffer,
> + READ_LINE_START);
> + }
> + else {
> + read_buffer = checked_realloc(read_buffer,
> + BYTES_TO_WORDS(read_buf_size
> + * sizeof(Char)));
Ditto for checked_realloc().
> + }
> + }
> + }
> + if (Res == 0) {
> + incr_hp_atomic(String, (i + sizeof(Word)) / sizeof(Word));
> + memcpy((char *)String, read_buffer, i * sizeof(Char));
> + ((Char *)String)[i] = '\\0';
If you're going to copy/assign ((i + 1) * sizeof(Char)) bytes, then you
should allocate that many. Also, it might be clearer to cast String to
`void *' or `Char *' rather than `char *'.
incr_hp_atomic(String, BYTES_TO_WORDS((i + 1) * sizeof(Char)));
memcpy((void *)String, read_buffer, i * sizeof(Char));
((Char *) String)[i] = '\\0';
> + }
> + else {
Should be "} else {", not on two lines.
I think you should add a test case to test reading of long lines.
Quite a few changes there, so I guess we should all stay tuned for round 3 ;-)
--
Fergus Henderson <fjh at cs.mu.oz.au> | "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh> | of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3 | -- the last words of T. S. Garp.
More information about the developers
mailing list