[m-rev.] io.m latest full diff

[Fergus Henderson]

On 21-Jan-2004, James Goddard <goddardjames at yahoo.com> wrote:
> +++ io.m	21 Jan 2004 04:52:26 -0000
> @@ -1168,6 +1168,11 @@
>       % io__make_temp(Name, IO0, IO) creates an empty file
>       % whose name which is different to the name of any existing file.
>       % Name is bound to the name of the file.
>       % On Microsoft Windows systems, the file will reside in the current
>       % directory if the TMP environment variable is not set, or in the
>       % directory specified by TMP if it is set.
>  	% On other systems, the file will reside in /tmp if the TMPDIR
>  	% environment variable is not set, or in the directory specified
>  	% by TMPDIR if it is set.
> +	% For the Java implementation, the system-dependent default
> +	% temporary-file directory will be used, specified by the system
> +	% property java.io.tmpdir. On UNIX systems the default value of this
> +	% property is typically "/tmp" or "/var/tmp"; on Microsoft Windows
> +	% systems it is typically "c:\\temp". 
>  	% It is the responsibility of the program to delete the file
>  	% when it is no longer needed.

I suggest the following:

	% io__make_temp(Name, IO0, IO) creates an empty file
	% whose name which is different to the name of any existing file.
	% Name is bound to the name of the file.
	% It is the responsibility of the program to delete the file
	% when it is no longer needed.
	%
	% The file will reside in an implementation-dependent directory.
	% For current Mercury implementations, it is determined as follows:
	% 1. For the non-Java back-ends:
	%    - On Microsoft Windows systems, the file will reside in
	%      the current directory if the TMP environment variable
	%      is not set, or in the directory specified by TMP if it is set.
	%    - On Unix systems, the file will reside in /tmp if the TMPDIR
	%      environment variable is not set, or in the directory specified
	%      by TMPDIR if it is set.
	% 2. For the Java back-end, the system-dependent default
	%    temporary-file directory will be used, specified by the Java
	%    system property java.io.tmpdir. On UNIX systems the default
	%    value of this property is typically "/tmp" or "/var/tmp";
	%    on Microsoft Windows systems it is typically "c:\\temp".

> @@ -4615,6 +4818,659 @@
>  }
>  ").
>  
> +:- pragma foreign_code("Java",
> +"
> +	// stdin, stdout, stderr and all text files are run through stream
> +	// readers/writers, which use the default charset encoding.  In other

I suggest "all text files (including stdin/stdout/stderr) ...".

> +	// words, while strings and characters are stored internally using
> +	// unicode, all files are read and written using the default system
> +	// charset.
> +	// Binary files are opened via RandomAccessFile, which supports seek,
> +	// but not character encoding/decoding or buffering.

You should mention the treatment of binary stdin/stdout here.

> +	public static class MR_MercuryFileStruct {
> +		public	static int			ML_next_stream_id = 0;
> +		public	int				id;
> +
> +		public	int				line_number	= 1;

Please document that this is only valid for text streams.

> +		private	java.util.Stack			pushback	= null;

Please document that this is non-null only for input streams.

> +		private java.io.InputStreamReader	input		= null;

Please document that this is non-null only for text input streams.

> +		private java.io.OutputStreamWriter	output		= null;

Please document that this is non-null only for text output streams.

> +		private java.io.RandomAccessFile	randomaccess	= null;

Please document that this is non-null only for binary streams other than
stdin/stdout.

> +		int					position	= 0;
> +		private java.lang.Object		channel		= null;

Please document what those fields are for, and when it is valid.

> +		/*
> +		** seek():
> +		**	seek relative to start, current position or end
> +		**	depending on the flag.
> +		**	This function only works for binary files.
> +		**	The binary versions of stdin and stdout are treated
> +		**	specially.  For java version 1.4, Reflection is used to
> +		**	obtain and use the neccessary FileChannel object needed
> +		**	to perform seeking on each.
> +		**	For older versions, seeking is rather limited.
> +		**	For mercury_stdin_binary, seek may be
> +		**	only be done forwards from the current position and for
> +		**	mercury_stdout_binary seek is not supported at all.

s/version 1.4/versions >= 1.4/

Also, it is not clear from the comment whether the limitations on seeking
of mercury_stdin/out_binary apply to all Java versions or only to older
versions.

> +		public void seek(int flag, int offset) {
> +			if (channel != null) {
> +				channelSeek(flag, offset);
> +				return;
> +			}
> +
> +			if (input != null || output != null) {
> +				throw new java.lang.RuntimeException(
> +						""text streams are not "" +
> +						""seekable"");
> +			}
> +			if (binary_output != null) {
> +				throw new java.lang.RuntimeException(
> +						""mercury_stdout_binary is "" +
> +						""not seekable"");
> +			}
> +			if (binary_input != null &&
> +					(flag != SEEK_CUR || offset < 0))
> +			{
> +				throw new java.lang.RuntimeException(
> +						""mercury_stdin_binary may "" +
> +						""only seek forwards from "" +
> +						""the current position"");
> +			}

The error messages here should probably be changed to make it specific
about which back-end(s) and which Java version(s) they apply to, e.g.

	Java text streams are not seekable

	for Java versions < 1.4, mercury_stdout_binary is not seekable

	for Java versions < 1.4, mercury_stdin_binary may only seek
	forwards from the current position

> +		private void channelSeek(int flag, int offset) {

Please add some comments about what this function does,
and what its preconditions are.

> +				Class[] argTypes = {Long.TYPE};
> +				java.lang.reflect.Method seek_mth =
> +						channel.getClass().getMethod(
> +						""position"", argTypes);
> +
> +				Object[] args = {new Long(position)};
> +				seek_mth.invoke(channel, args);

Please add a comment here explaining why reflection is used.

> +		/*
> +		** read_char():
> +		**	Reads one character in from a text input file using the
> +		**	default charset decoding.  Returns -1 at end of file.
> +		*/
> +		public int read_char() {
> +			int c;
> +			if (input == null) {
> +				throw new java.lang.RuntimeException(
> +					""read_char_code may only be called"" +
> +					"" on text input streams"");

This code here, and quite a bit of code elsewhere, is going to break if
text mode operations are attempted on a binary file.  The implementations
of io__read_binary and io__write_binary do exactly that.

Fixing this problem would require a major redesign, so I suggest that
for now, you just document the problem with XXX comments, both at the
definition of the MercuryFile structure and also at the definition
of io__read_binary/io__write_binary.

> +:- pred command_line_argument(int, string).
> +:- mode command_line_argument(in, out) is semidet.
> +
> +	% XXX This predicate is currently only used by the Java implementation,
> +	% but to prevent compilation warnings for (eg) the C implementation,
> +	% some definition needs to be present.
> +command_line_argument(_, "") :-
> +	semidet_fail.

I would recommend writing that as

	command_line_argument(_, "") :-
		( semidet_succeed ->
			error("unexpected call to command_line_argument")       
		;
			semidet_fail
		).

so that it will report a run-time error rather than silently failing
if it is called unexpectedly.

Otherwise that looks fine.

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