diff: io__read_line_as_string (second round)

[Fergus Henderson]

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.