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

[Fergus Henderson]

On 11-Aug-2000, Peter Ross <peter.ross at miscrit.be> wrote:
> Add a variant of the MercuryFile structure that it 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.
> 
> This variant is turned on by passing --enable-new-mercuryfile-struct to
> configure.

OK, the basic structure of this diff looks good.
So this time I'm going through the code in a bit more detail.

...
> +++ logged_output.m	Fri Aug 11 21:01:59 2000
> +:- pragma c_code(create_stream(FileName::in, IOStream::out, IO0::di, IO::uo), "
> +	MercuryFile	*stream;
> +	FILE		*file;
> +
> +	file = fopen(FileName, ""w"");
> +
> +	incr_hp(stream, sizeof(MercuryFile));

The argument to incr_hp should be a size in Words, not a size in bytes.

> +	IOStream = (Word) stream;

s/Word/MR_Word/

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

s/pszFormat/format/

Even Microsoft have now realized that Hungarian notation was a mistake ;-) 
The new .NET stuff does not use Hungarian notation.

> +int
> +ME_logged_output_write(MR_StreamInfo *info, const void *buffer, size_t size)
> +{
> +	int rc;							       
> +	fwrite(buffer, sizeof(unsigned char), size, stdout);
> +	rc = fwrite(buffer, sizeof(unsigned char), size, info->file);
> +}

You need `return rc;' at the end of this function.

io.m:
> Index: library/io.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/io.m,v
> retrieving revision 1.200
> diff -u -r1.200 io.m
> --- library/io.m	2000/08/01 09:04:19	1.200
> +++ library/io.m	2000/08/11 11:02:18
> @@ -1494,7 +1494,7 @@
>  		[will_not_call_mercury, thread_safe],
>  "{
>  	MercuryFile *f = (MercuryFile *) Stream;
> -	clearerr(f->file);
> +	clearerr(MR_file(*f));
>  }").

Hmm, there are lots of places like this that access
`MR_file' without checking what kind of file it is.
If the file here is a user-defined file, then it
might not have an associated `FILE *', in which case
this code will have undefined behaviour, which is not good.
I think you need to go through all the code looking for
occurrences of MR_file(), and make them all do something
reasonable in the case where MR_IS_FILE_STREAM() returns false.

> @@ -1650,7 +1662,12 @@
>  	char *buffer = (Char *) Buffer0;
>  	int items_read;
>  
> -	items_read = fread(buffer + Pos0, sizeof(Char), Size - Pos0, f->file);
> +	if (MR_IS_FILE_STREAM(*f)) {
> +		items_read = fread(buffer + Pos0, sizeof(Char), Size - Pos0,
> +				MR_file(*f));
> +	} else {
> +		mercury_io_error(f, ""Attempted fread from non-file stream"");

Why do you call mercury_io_error() here?
Rather than this if-then-else, shouldn't you just call the `MR_READ()' macro?

> +  int
> +  MR_write(MR_StreamInfo *info, const void *buffer, size_t size)
> +  {
> +	int rc;							       
> +
> +	MR_assert(info != NULL);
> +	rc = fwrite(buffer, sizeof(unsigned char), size, info->file);
> +
> +  }

You need a `return rc;' at the end of that function.
Or get rid of the `rc' variable entirely, and do `return fwrite(...);'.

> +++ runtime/mercury_library_types.h	2000/08/11 11:02:23
...
> +  #define MR_UNGETCH(mf, ch)	ungetc((int) (ch), MR_file(mf))

Why the cast to `(int)'?

I'd like to see another diff when you've addressed those issues.  Thanks.

-- 
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
--------------------------------------------------------------------------