[m-dev.] for review: change MercuryFile structure

[Fergus Henderson]

On 28-Jul-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> For Fergus to review.
> 
> I will do the changes required to io.m to handle bidirectional streams
> as a seperate change, as they are independent of this change.
> 
> ===================================================================
> 
> Estimated hours taken: 20
> 
> Change the MercuryFile structure so that it now contains pointers to
> functions which operate on the MercuryFile.  This allows us, for
> example, to create a MercuryFile structure which operates on sockets and
> use the predicates in io.m to operate on the socket stream.

OK, so this change is another version of the one previously
discussed in the thread "Add Mission Critical extensions".
Unlike the previous version, in this version the changes
are applied unconditionally, rather than being #ifdef'd.

The main drawback of that is the possible performance
implications.  What's the performance impact of this change?
For example, how much does this change slow down `cat'
programs written using io__read_char or io__read_line_as_string?
I don't think we should commit this change until we have
measured the performance impact.  If the performance impact
is too high, then we should go with the conditional compilation
approach.

> +++ mercury_file.c	Fri Jul 28 21:43:00 2000
> @@ -0,0 +1,116 @@
> +/*
> +** Copyright (C) 2000 The University of Melbourne.
> +** This file may only be copied under the terms of the GNU Library General
> +** Public License - see the file COPYING.LIB in the Mercury distribution.
> +*/
> +
> +/*
> +** This module contains the implementation of C `FILE *' streams.

That comment makes it sound like this module is part of the C
standard library implementation.

I suggest you reword it e.g. as

	This module contains an implementation of Mercury streams
	using C `FILE *' streams.

> +int
> +MR_getch(MR_StreamInfo *info) 
> +{
> +	int ch;
> +
> +	assert(info!=NULL);
> +	ch = getc(info->file);	  
> +	return ch;
> +}

s/!=/ != /	(likewise in many other places in this file)
     ^  ^

You should use `MR_assert()' rather than `assert()' here.
Likewise for all of the other uses of `assert()'.

The reason is that we can't easily disable assert()
for code in the Mercury runtime without affecting
calls to assert() in user C code (e.g. `pragma c_code').

(Note that `MR_assert()' is disabled by default.
That's fine in this case, since on modern machines
dereferencing a null pointer will trap.  Your calls
to assert() here are basically just documentation.)

The `ch' variable is not needed; it would be more concise
to write that as just

	return getc(info->file);

and doing so would be more consistent with the code
for MR_putc(), MR_close(), etc.

> +	  
> +int
> +MR_ungetch(MR_StreamInfo *info, int ch)
> +{
> +	int res;
> +	assert(info!=NULL);		
> +	if ((res = ungetc((int) ch, info->file)) == EOF) {

The cast to `(int)' here is not needed.

Some people would consider it clearer to write that in two statements:

	res = ungetc(ch, info->file);
	if (res == EOF) {

> +	        fprintf(stderr, "io__putback_char2(%c,%d): ungetc failed\n",
> +				res, (int) ch);
> +	}

That format string looks wrong.
I don't understand why you both printing out `res' at all,
since you know that its value can only be EOF.
And I certainly don't understand why you print it out as `%c'.
And what does `io__putback_char2' have to do with it?

Furthermore, if something like that goes wrong, you shouldn't
just print a message to stderr; instead, you should throw an
exception, like the current code in io.m does.

> +	return (int) res;

The cast to (int) here is not needed.

> +++ mercury_file.h	Fri Jul 28 21:43:01 2000
> +int MR_ungetch(MR_StreamInfo *info,int);

s/,int/, int/

> +int MR_vfprintf(MR_StreamInfo *info, const char *pszFormat, va_list ap);

s/pszFormat/format/

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------