[m-rev.] For review: Java implementation of IO library

[Fergus Henderson]

On 08-Jan-2004, James Goddard <goddardjames at yahoo.com> wrote:
> 
> Implement some library procedures for the Java back end.
> 
> Index: io.m
...
> +:- pragma foreign_proc("Java",
> +	io__file_modification_time_2(FileName::in, Status::out, Msg::out,
> +		Time::out, _IO0::di, _IO::uo),
> +	[will_not_call_mercury, promise_pure, tabled_for_io, thread_safe],
> +"
> +	Time = new java.util.Date();
> +
> +	try {
> +		((java.util.Date) Time).setTime(
> +				(new java.io.File(FileName)).lastModified());
> +		Msg = """";
> +		Status = 1;
> +	} catch (java.lang.Exception e) {
> +		((java.util.Date) Time).setTime(0);
> +		Msg = ""lastModified() failed: "" + e.getMessage();
> +		Status = 0;
> +	}
> +").

It would be nicer to avoid the casts:

	java.util.Date date = new java.util.Date();
	try {
		date.setTime((new java.io.File(FileName)).lastModified());
		Msg = """";
		Status = 1;
	} catch (java.lang.Exception e) {
		date.setTime(0);
		Msg = ""lastModified() failed: "" + e.getMessage();
		Status = 0;
	}
	Time = date;

Also, the call to setTime(0) is unnecessary; you might as well delete that.

According to the docs, lastModified() returns 0 if the file does not exist
or if an I/O error occurs.  You should check the return value from
lastModified(), and if it is zero, Status should be set to 0, not 1.

> +:- pragma foreign_proc("Java",
> +	io__file_type_2(_FollowSymLinks::in, FileName::in,
> +		Result::out, _IO0::di, _IO::uo),
> +	[may_call_mercury, promise_pure, tabled_for_io, thread_safe],
> +"
> +	java.io.File file = new java.io.File(FileName);
> +
> +	// The Java implementation can distinguish between regular files and
> +	// directorys, and for everything else it just returns unknown.

s/ys/ies/

> @@ -4603,6 +4796,257 @@
>  }
>  ").
>  
> +:- pragma foreign_code("Java",
> +"
> +	/* stdin, stdout, stderr treated as input/output streams,
> +	** all files to use random access r+w

That's OK as a quick hack to get things working, I guess, but I think it's
not adequate in the long run.  If you open all files r+w, it will result
in permission errors when trying to read a read-only file, won't it?
So there should at least be an XXX comment there.

Please also check that it is possible to open files which are not seekable,
e.g. pipes, and if it is not, then at least put an XXX comment.

> +		public MR_MercuryFileStruct(java.lang.String file) {
> +			try {
> +				randomaccess	= new java.io.RandomAccessFile(
> +						file, ""rws"");

Please put a comment here explaining the meaning of "rws".  Also, there
should be a comment explaining why "rws" was chosen rather than "rw" or
"rwd".

> +			} catch (java.lang.Exception e) {
> +				throw new RuntimeException(e.getMessage());
> +			}

Please put a comment here explaining why this is not just "throw e".

> +		public MR_MercuryFileStruct(java.lang.String file,
> +				boolean append)
> +		{
> +			try {
> +				randomaccess	= new java.io.RandomAccessFile(
> +						file, ""rws"");
> +				id		= ML_next_stream_id++;
> +				mode		= BOTH;
> +				pushback	= new java.util.Stack();
> +				filename	= file;
> +				line_number	= 1;
> +
> +				if (append) {
> +					while (true) {
> +						int c = randomaccess.read();
> +						if (c == '\\n') {
> +							line_number++;
> +						} else if (c == -1) {
> +							break;
> +						}
> +					}
> +				// XXX hopefully at this point we're at the
> +				// end of the file, ready to append.
> +				}

That looks inefficient.  Better to just seek to the end of file,
i.e. seek(SEEK_END, 0).

The behaviour of the line number for files opened in append mode should
be documented in the interface section of io.  The Java back-end should
do the same thing as the C back-end in that regard.

I think for the C back-end we only count lines written after the file
was opened.  This is more efficient than trying to count the number of
lines in the file.

> +		public void putc(int c) {
> +			if (mode == INPUT) {
> +				throw new java.lang.RuntimeException(
> +					""Attempted to write to input stream"");
> +			}
> +			if (c == '\\n') {
> +				line_number++;
> +			}
> +	
> +			try {
> +				if (output != null) {
> +					output.write(c);
> +				} else {
> +					randomaccess.write(c);
> +				}
> +			} catch (java.io.IOException e) {
> +				throw new java.lang.RuntimeException(
> +						e.getMessage());
> +			}
> +			pushback = new java.util.Stack();
> +		}

Why does putc() reset the pushback?
(And likewise for write().)

> +		public void flush() {
> +			if (mode == INPUT) {
> +				throw new java.lang.RuntimeException(
> +					""Attempted to flush input stream"");
> +			}
> +
> +			try {
> +				if (output != null) {
> +					output.flush();
> +				}
> +				// else randomaccess already writes all data
> +				// synchronously to the underlying device.
> +			} catch (java.io.IOException e) {
> +				throw new java.lang.RuntimeException(
> +						e.getMessage());
> +			}
> +		}

Since the Mercury interface has an explicit flush operation,
wouldn't it be better to use "rw" rather than "rws", and
rely on the user calling flush appropriately if needed?

There's obviously an efficiency versus reliability trade-off here;
but is there any reason why we should do it differently for Java
than for C?

> +:- pragma foreign_code("Java",
> +"
> +static MR_MercuryFileStruct mercury_open(String filename, String openmode) {
> +	try {
> +		if (openmode.charAt(0) == 'r') {
> +			return new MR_MercuryFileStruct(filename);
> +		}
> +		if (openmode.charAt(0) == 'w') {
> +			return new MR_MercuryFileStruct(filename);
> +		}

Those two operations have different semantics.
If openmode = "r", then the operation should succeed so long as the
file is readable, even if it is read-only.  If openmode = "w", 
the operation should succeed so long as the file is writable,
even if it is write-only.

[to be continued...]

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
The University of Melbourne         |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- the last words of T. S. Garp.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------