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

[Peter Ross]

On Fri, Aug 11, 2000 at 10:08:08PM +1000, Fergus Henderson wrote:
> 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.
> 
You got something against hungarians? ;) 


> > +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.
> 
It is there, I don't now why it wasn't in the diff.


> 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.
> 
Only functions in io.m will possible be called with possibly non FILE *
pointers.  I have added in checks for all the io.m functions.


> > @@ -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?
> 
Yes that would be better.

> > +  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(...);'.
> 
Again it was there I don't know why it wasn't in the diff (note it was
exactly the same line as the other one).

> > +++ 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)'?
> 
Because ungetc expects an int as its first argument.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------