[m-dev.] for review: Java backend

[Fergus Henderson]

On 14-Feb-2001, Julien Fischer <juliensf at students.cs.mu.oz.au> wrote:
> 
> +mlds_output_initializer(Type, Initializer) -->
...
> +		%
> +		% If we are not provided with an initializer we just, 
> +		% supply the default java values -- note: this is strictly
> +		% only necessary for local variables, but it's not going
> +		% to hurt anything else.  
> +		% The reason for doing this is to prevent the Java compiler
> +		% failing due to definite assignment.  This happens in
> +		% switch statements where the default case is unreachable
> +		% and we terminate execution at that point.  The compiler's
> +		% definite assignment analysis cannot take this into account
> +		% and declares this to be an error.
> +		% XXX We should catalog exactly which constructs are causing
> +		%     this to happen.
> +		% XXX If we ever need to use final local variables this
> +		%     will need to be fixed.
> +		%
> +		io__write_string(get_java_type_initializer(Type))

That's not the right fix for that problem.
The right fix is to use `throw' to terminate execution.
Then Java's rules for definite assignment will be satisfied,
since it will know that the `throw' can't fall through.

> +mlds_output_initializer_body(init_struct(FieldInits)) --> 
> +	io__write_list(FieldInits, ",\n\t\t", 
> +		mlds_output_initializer_body).

Do you need to wrap curly braces around the fields, like you do
for init_array?

> +:- pred mlds_output_func_decl(indent, qualified_entity_name, mlds__context,
> +		func_params, io__state, io__state).
> +:- mode mlds_output_func_decl(in, in, in, in, di, uo) is det.
> +
> +mlds_output_func_decl(Indent, QualifiedName, Context, Signature) -->
> +	mlds_output_func_decl_ho(Indent, QualifiedName, Context, Signature,
> +			mlds_output_type, mlds_output_type_suffix).
> +
> +:- pred mlds_output_func_decl_ho(indent, qualified_entity_name, mlds__context,
> +		func_params, output_type, output_type, io__state, io__state).
> +:- mode mlds_output_func_decl_ho(in, in, in, in, in(output_type),
> +		in(output_type), di, uo) is det.
> +
> +mlds_output_func_decl_ho(Indent, QualifiedName, Context, Signature,
> +		OutputPrefix, OutputSuffix) -->
> +	{ Signature = mlds__func_params(Parameters, RetTypes) },
> +	( { RetTypes = [] } ->
> +		io__write_string("void")
> +	; { RetTypes = [RetType] } ->
> +		OutputPrefix(RetType)
> +	;
> +		% for multiple outputs, we return an array of objects.
> +		io__write_string("java.lang.Object []")
> +	),
> +	io__write_char(' '),
> +	{ QualifiedName = qual(ModuleName, Name) },
> +	mlds_output_name(Name),	
> +	mlds_output_params(OutputPrefix, OutputSuffix,
> +			Indent, ModuleName, Context, Parameters),
> +	( { RetTypes = [RetType2] } ->
> +		OutputSuffix(RetType2)
> +	;
> +		[]
> +	).

Again, all the stuff with OutputSuffix should go away, since it is not
needed for Java.  I don't think you need to split the code for
outputting function declarations into mlds_output_func_decl and
mlds_output_func_decl_ho either.

(Indent, QualifiedName, Context, Signature) -->

> +:- pred mlds_output_param(output_type, output_type,
> +		indent, mlds_module_name, mlds__context,
> +		pair(mlds__entity_name, mlds__type), io__state, io__state).
> +:- mode mlds_output_param(in(output_type), in(output_type),
> +		in, in, in, in, di, uo) is det.
> +
> +mlds_output_param(OutputPrefix, OutputSuffix, Indent,
> +		ModuleName, Context, Name - Type) -->
> +	mlds_indent(Context, Indent),
> +	mlds_output_data_decl_ho(OutputPrefix, OutputSuffix,
> +			qual(ModuleName, Name), Type).

Same comments apply here.
I don't think the separation into * and *_ho is needed here
either.  Also the OutputSuffix stuff should go.
And I don't think you need to pass OutputPrefix, since
that is only ever set to one value AFAIK.

> +:- pred mlds_output_func_type_prefix(func_params, io__state, io__state).
> +:- mode mlds_output_func_type_prefix(in, di, uo) is det.
> +
> +mlds_output_func_type_prefix(Params) -->
> +	{ Params = mlds__func_params(_Parameters, RetTypes) },
> +	( { RetTypes = [] } ->
> +		io__write_string("void")
> +	; { RetTypes = [RetType] } ->
> +		mlds_output_type(RetType)
> +	;
> +		{ error("mlds_output_func_type_prefix: multiple return types") }
> +	).
> +
> +:- pred mlds_output_func_type_suffix(func_params, io__state, io__state).
> +:- mode mlds_output_func_type_suffix(in, di, uo) is det.
> +
> +mlds_output_func_type_suffix(Params) -->
> +	{ Params = mlds__func_params(Parameters, _RetTypes) },
> +	io__write_string(")"),
> +	mlds_output_param_types(Parameters).

I don't think that code makes much sense for Java, since Java doesn't
have syntax for function types.

> +%-----------------------------------------------------------------------------%
> +%
> +% Code to output names of various entities
> +% XXX Much of the code in this section will not work when we 
> +%     start enforcing names properly.
> +
> +:- pred mlds_output_fully_qualified_name(mlds__qualified_entity_name,
> +		io__state, io__state).
> +:- mode mlds_output_fully_qualified_name(in, di, uo) is det.
> +
> +mlds_output_fully_qualified_name(QualifiedName) -->
> +	{ QualifiedName = qual(_ModuleName, Name) },
> +	(
> +		(
> +			{ Name = export(_) } 
> +		;
> +			{ Name = data(_) }
> +		)
> +	->
> +		mlds_output_name(Name)
> +	;
> +		mlds_output_fully_qualified(QualifiedName, mlds_output_name)
> +	).

You should explain why the module qualifier is ignored for
export/1 and data/1.

> +:- pred mlds_output_module_name(mercury_module_name, io__state, io__state).
> +:- mode mlds_output_module_name(in, di, uo) is det.
> +
> +mlds_output_module_name(ModuleName) -->
> +	{ llds_out__sym_name_mangle(ModuleName, MangledModuleName) },
> +	io__write_string(MangledModuleName).

That looks wrong; llds_out__sym_name_mangle will qualify names
with `__', but for Java they should be qualified with `.'.

If this is needed at all, it should probably be named something
different, e.g. `output_module_name_as_single_identifier'
or `output_mangled_module_name' or something like that.
But I think you need to look at all of the callers to this
to see whether it is appropriate to use `.' or `__'.

You should try some of the tests of nested modules (e.g.
see tests/hard_coded/sub-modules), and if they don't succeed,
then you should add nested modules to the list of things that
don't work.

> +mlds_output_type(mlds__ptr_type(Type)) -->
> +	({ Type = mlds__class_type(Name, Arity, _Kind) } ->

s/({/( {/

> +mlds_output_type(mlds__func_type(FuncParams)) -->
> +	mlds_output_func_type_prefix(FuncParams).

I don't think that is right.
Better at least add an XXX here.
It might be better to just do something like
	
	mlds_output_type(mlds__func_type(FuncParams)) -->
		% XXX not yet implemented
		io__write_string("\n*** mlds_output_type: sorry, not implemented: "),
		io__print(mlds__func_type(FuncParams)), io__nl, io__nl.

> +mlds_output_mercury_user_type(Type, TypeCategory) -->
> +		( { type_to_type_id(Type, TypeId, _ArgsTypes) } ->
> +			{ ml_gen_type_name(TypeId, ClassName, ClassArity) },
> +			{ TypeCategory = enum_type ->
> +				MLDS_Type = mlds__class_type(ClassName,
> +					ClassArity, mlds__enum)
> +			;
> +				MLDS_Type = mlds__class_type(
> +					ClassName, ClassArity, mlds__class)
> +			},
> +			mlds_output_type(MLDS_Type)
> +		;
> +			{ error("mlds_output_mercury_user_type") }
> +		).

Fix the indentation.

> +	%
> +	% XXX This should be renamed, but I'm not sure to what.
> +	%
> +:- pred mlds_output_type_suffix(mlds__type, io__state, io__state).
> +:- mode mlds_output_type_suffix(in, di, uo) is det.
> +
> +mlds_output_type_suffix(Type) -->
> +	mlds_output_type_suffix(Type, no_size).

It shouldn't just be renamed, it should be eliminated.

> +:- type initializer_array_size
> +	--->	array_size(int)
> +	;	no_size.	
> +
> +:- func initializer_array_size(mlds__initializer) = initializer_array_size.
> +initializer_array_size(no_initializer) = no_size.
> +initializer_array_size(init_obj(_)) = no_size.
> +initializer_array_size(init_struct(_)) = no_size.
> +initializer_array_size(init_array(Elems)) = array_size(list__length(Elems)).

That can be deleted, since it is not used.

> +:- pred mlds_output_type_suffix(mlds__type, initializer_array_size,
> +		io__state, io__state).
> +:- mode mlds_output_type_suffix(in, in, di, uo) is det.
> +
> +mlds_output_type_suffix(mercury_type(_, _), _) --> [].
> +mlds_output_type_suffix(mlds__native_int_type, _) --> [].
> +mlds_output_type_suffix(mlds__native_float_type, _) --> [].
> +mlds_output_type_suffix(mlds__native_bool_type, _) --> [].
> +mlds_output_type_suffix(mlds__native_char_type, _) --> [].
> +mlds_output_type_suffix(mlds__class_type(_, _, _), _) --> [].
> +mlds_output_type_suffix(mlds__ptr_type(_), _) --> [].
> +mlds_output_type_suffix(mlds__array_type(_), _ArraySize) --> [].
> +mlds_output_type_suffix(mlds__func_type(FuncParams), _) -->
> +	mlds_output_func_type_suffix(FuncParams).
> +mlds_output_type_suffix(mlds__generic_type, _) --> [].
> +mlds_output_type_suffix(mlds__generic_env_ptr_type, _) --> [].
> +mlds_output_type_suffix(mlds__pseudo_type_info_type, _) --> [].
> +mlds_output_type_suffix(mlds__commit_type, _) --> [].
> +mlds_output_type_suffix(mlds__unknown_type, _) -->
> +	{ error("mlds_to_java.m: suffix has unknown type") }.

Those all do nothing, so they can be deleted easily enough...

> +mlds_output_type_suffix(mlds__cont_type(ArgTypes), _) -->
> +	( { ArgTypes = [] } ->
> +		[]
> +	;
> +		% This case only happens for --nondet-copy-out
> +		io__write_string(")("),
> +		io__write_list(ArgTypes, ", ", mlds_output_type),
> +		io__write_string(")")
> +	).

That code doesn't work; what it outputs is the C syntax for
a function type, but Java doesn't have syntax for function types.

These will eventually need to be handled similarly to function types.

> +mlds_output_type_suffix(mlds__rtti_type(RttiName), ArraySize) -->
> +	( { rtti_name_has_array_type(RttiName) = yes } ->
> +		mlds_output_array_type_suffix(ArraySize)
> +	;
> +		[]
> +	).
> +
> +:- pred mlds_output_array_type_suffix(initializer_array_size::in,
> +		io__state::di, io__state::uo) is det.
> +mlds_output_array_type_suffix(no_size) -->
> +	io__write_string("[]").
> +mlds_output_array_type_suffix(array_size(_Size)) -->
> +	io__write_string("[]").  % For Java we don't need to know the size.

For Java the "[]" can be in the type prefix rather than the type suffix.
Doing so would make this code quite a bit simpler, so it should be done.

> +:- pred mlds_output_access(access, io__state, io__state).
> +:- mode mlds_output_access(in, di, uo) is det.
> +
> +mlds_output_access(public)    --> io__write_string("public ").
> +mlds_output_access(private)   --> io__write_string("private ").
> +mlds_output_access(protected) --> io__write_string("protected ").
> +mlds_output_access(default)   --> []. 
> +mlds_output_access(local)     --> [].

I suggest outputting "/* default */ " for the `default' case.

> +:- pred mlds_output_virtuality(virtuality, io__state, io__state).
> +:- mode mlds_output_virtuality(in, di, uo) is det.
> +
> +mlds_output_virtuality(virtual)     --> io__write_string("abstract ").
> +mlds_output_virtuality(non_virtual) --> [].

That is wrong.  "abstract" doesn't mean the same thing as "virtual".

> +:- pred mlds_output_finality(finality, io__state, io__state).
> +:- mode mlds_output_finality(in, di, uo) is det.
> +
> +mlds_output_finality(final)       --> []. 
> +mlds_output_finality(overridable) --> [].

That is wrong, I think.  For `final', the obvious thing to do for Java
would be to output "final".  If not, then at very least there should
be a comment here explaining why not.

> +:- pred mlds_output_constness(constness, io__state, io__state).
> +:- mode mlds_output_constness(in, di, uo) is det.
> +
> +mlds_output_constness(const)      --> [].
> +mlds_output_constness(modifiable) --> [].

It might be a good idea to output `/* const */ ' for the `const' case,
at least in the case when --auto-comments is enabled.

> +:- pred mlds_output_abstractness(abstractness, io__state, io__state).
> +:- mode mlds_output_abstractness(in, di, uo) is det.
> +
> +mlds_output_abstractness(abstract) --> []. 
> +mlds_output_abstractness(concrete) --> [].

Now *here* looks like the right place to output "abstract ";
if not, you should document why not.

> +	%
> +	% selection (if-then-else)
> +	%
> +mlds_output_stmt(Indent, FuncInfo, if_then_else(Cond, Then0, MaybeElse),
> +		Context) -->
> +	%
> +	% we need to take care to avoid problems caused by the
> +	% dangling else ambiguity
> +	%
> +	{
> +		%
> +		% For examples of the form
> +		%
> +		%	if (...)
> +		%		if (...)
> +		%			...
> +		%	else
> +		%		...
> +		%
> +		% we need braces around the inner `if', otherwise
> +		% they wouldn't parse they way we want them to:
> +		% Java would match the `else' with the inner `if'
> +		% rather than the outer `if'.
> +		%
> +		MaybeElse = yes(_),
> +		Then0 = statement(if_then_else(_, _, no), ThenContext)
> +	->
> +		Then = statement(block([], [Then0]), ThenContext)
> +	;
> +		%
> +		% For examples of the form
> +		%
> +		%	if (...)
> +		%		if (...)
> +		%			...
> +		%		else
> +		%			...
> +		%
> +		% we don't _need_ braces around the inner `if',
> +		% since Java will match the else with the inner `if'.
> +		%

This seems to be cut-and-paste from equivalent code in mlds_to_c.m,
but the comment in mlds_to_c.m ended with

		% but we add braces anyway, to avoid a warning from gcc.

You seem to be adding the braces anyway, so you should say so in the
comment, and it would be helpful to explain why (perhaps to make
the generated code easier to read?).

> +	%
> +	% transfer of control
> +	% 
> +mlds_output_stmt(_Indent, _FuncInfo, label(_LabelName), _) --> 
> +	{ error("mlds_to_java.m: Labels not supported in Java.") }.
> +mlds_output_stmt(_Indent, _FuncInfo, goto(_LabelName), _) --> 
> +	{ error("mlds_to_java.m: gotos not supported in Java.") }.

Be consistent about the capitalization: s/Labels/labels/

Use unexpected/2 and this_file/0 rather than error/1.

> +mlds_output_stmt(_Indent, _FuncInfo, computed_goto(_Expr, _Labels), 
> +	_Context) --> 
> +	{ error("mlds_to_java.m: computed gotos not supported in Java.") }.

Likewise here.

> +mlds_output_stmt(Indent, FuncInfo, return(Results), _) -->
...
> +	; { Results = [Rval] } ->
> +		io__write_char(' '),
> +		% XXX the ml code generator should not generate these.
> +		( { Rval = mlds__lval(Lval),
> +		    Lval = var(VarName, _),
> +		    VarName = qual(_, UnqualName),
> +		    UnqualName = "dummy_var" } 
> +		
> +		->
> +			[]
> +		;
> +			mlds_output_rval(Rval)
> +		)

Fix the layout of the condition of that if-then-else.

Why is that code needed? 
When does the ml code generator generate such assignments?

> +%=============================================================================%
> +% XXX This hasn't been implemented yet in the Java backend.  The code
> +%      below is from the C backend.
> +
> +	%
> +	% commits
> +	%

These should be implemented using catch/throw.
(However, it doesn't have to be done as part of this change.)

> +:- pred mlds_output_case_cond(indent, mlds__context,
> +		mlds__case_match_cond, io__state, io__state).
> +:- mode mlds_output_case_cond(in, in, in, di, uo) is det.
> +
> +mlds_output_case_cond(Indent, Context, match_value(Val)) -->
> +	mlds_indent(Context, Indent),
> +	io__write_string("case "),
> +	mlds_output_rval(Val),
> +	io__write_string(":\n").
> +mlds_output_case_cond(Indent, Context, match_range(Low, High)) -->
> +	% This uses the GNU C extension `case <Low> ... <High>:'.
> +	mlds_indent(Context, Indent),
> +	io__write_string("case "),
> +	mlds_output_rval(Low),
> +	io__write_string(" ... "),
> +	mlds_output_rval(High),
> +	io__write_string(":\n").



> +:- pred mlds_output_switch_default(indent, func_info, mlds__context,
> +		mlds__switch_default, io__state, io__state).
> +:- mode mlds_output_switch_default(in, in, in, in, di, uo) is det.
> +
> +mlds_output_switch_default(Indent, _FuncInfo, Context, default_is_unreachable) -->
> +	mlds_indent(Context, Indent),
> +	io__write_string("default: /*NOTREACHED*/\n"), 
> +	mlds_indent(Context, Indent + 1),
> +	io__write_string("java.lang.System.exit(0);\n").

Use `throw' rather than calling exit().

> +%-----------------------------------------------------------------------------%
> +	%
> +	% Does the rval represent a special procedure for which a
> +	% code address doesn't exist.
> +	%
> +:- pred no_code_address(mlds__rval::in) is semidet.
> +
> +no_code_address(const(code_addr_const(proc(qual(Module, PredLabel - _), _)))) :-
> +	SymName = mlds_module_name_to_sym_name(Module),
> +	SymName = qualified(unqualified("mercury"), "private_builtin"),
> +	PredLabel = pred(predicate, _, "unsafe_type_cast", 2).

That procedure is wrong, and is also unused.  It should be deleted.

> +	%
> +	% assignment
> +	%
> +mlds_output_atomic_stmt(Indent, _FuncInfo, assign(Lval, Rval), _) -->
> +	mlds_indent(Indent),
> +	mlds_output_lval(Lval),
> +	io__write_string(" = "),
> +	( { Lval = var(_, VarType), 
> +	    type_is_object(VarType) } 
> +	    
> +	 ->

Fix the layout of that if-then-else.

> +		% If the Lval is a an object.	
> +		
> +		( { rval_is_int_const(Rval) } ->
> +			io__write_string("new "),
> +			mlds_output_type(VarType),
> +			io__write_string("("),
> +			mlds_output_rval(Rval),
> +			io__write_string(")")
> +		;		
> +			%( { rval_is_enum_var(Rval) } ->
> +				mlds_output_rval(Rval)
> +			%;
> +			%	io__write_string("("),
> +			%	mlds_output_type(VarType),
> +			%	io__write_string(") "),
> +			%	mlds_output_rval_maybe_with_enum(Rval)
> +			%)

You should explain why that code is commented out.

> +	%
> +	% heap management
> +	%
> +mlds_output_atomic_stmt(_Indent, _FuncInfo, delete_object(_Lval), _) -->
> +	{ error("mlds_to_java.m: delete_object not supported in Java.") }.

Why not just use `delete'?

	mlds_output_atomic_stmt(_Indent, _FuncInfo, delete_object(Lval), _) -->
		io__write_string("delete "),
		mlds_output_lval(Lval).

> +mlds_output_atomic_stmt(Indent, _FuncInfo, NewObject, Context) -->
> +	{ NewObject = new_object(Target, _MaybeTag, Type, _MaybeSize,
> +		MaybeCtorName, Args, ArgTypes) },
> +	
> +	mlds_indent(Indent),
> +	io__write_string("{\n"),
> +	mlds_indent(Context, Indent + 1),
> +	mlds_output_lval(Target),
> +	io__write_string(" = new "),
> +	%
> +	% All derived classes are also static member classes of their
> +	% bases classes.

Not true.  It's only true for the derived classes that the
Mercury compiler generates for constructors of discriminated
union types.  The comment should be reworded.

>         So the correct way to call their constructors
> +	% is `<module_name>.<base_class>.<derived_class>()'.
> +	% The names of the derived classes are stored in the MaybeCtorName
> +	% field.  
> +	% XXX We should actually generate (Java) contructors for each class.  
> +	% This would make the treatment of discriminated unions more consistent
> +	% with the way we treat enumeations.  At the moment we just assign the
> +	% values directly to the fields.

s/enumeations/enumerations/

> +	%
> +	( { MaybeCtorName = yes(QualifiedCtorId) } ->
> +		mlds_output_type(Type),
> +		io__write_char('.'),
> +		{ QualifiedCtorId = qual(_ModuleName, CtorDefn) },
> +		{ CtorDefn = ctor_id(CtorName, CtorArity) },
> +		{ llds_out__name_mangle(CtorName, MangledCtorName) },
> +		io__write_string(MangledCtorName),
> +		io__write_string("_"),
> +		io__write_int(CtorArity)
> +	;
> +		% XXX This probably isn't right.
> +		io__write_string("null"),
> +		{ CtorDefn = ctor_id("null", 0) }
> +	),
> +	io__write_string("();\n"),

`null()' definitely looks wrong to me.

When does that case occur?  Is there some reason why you don't just
call error/1 or unexpected/2 in that case?

> +	% Output initial values of an object's fields.
> +	%
> +:- pred mlds_output_init_args(list(mlds__rval), list(mlds__type), mlds__ctor_id, 		mlds__context, int, mlds__lval, mlds__tag, indent, io__state, 
> +		io__state).
> +:- mode mlds_output_init_args(in, in, in, in, in, in, in, in, di, uo) is det.

Fix the line wrap.

> +:- pred mlds_output_lval(mlds__lval, io__state, io__state).
> +:- mode mlds_output_lval(in, di, uo) is det.
> +
> +mlds_output_lval(field(_MaybeTag, _Rval, offset(_OffsetRval),
> +		_FieldType, _ClassType)) -->
> +	{ error("mlds_to_java.m : mlds_output_lval") }.

Use unexpected/2.

Use a better error message, e.g. "mlds_output_lval: offset field".
(Except the `mlds_' prefix should go, as mentioned previously.)

> +mlds_output_lval(field(_MaybeTag, PtrRval, named_field(FieldName, CtorType),
> +		_FieldType, _PtrType)) -->
> +	( { FieldName = qual(_, UnqualFieldName), 
> +	    llds_out__name_mangle(UnqualFieldName, MangledFieldName),
> +	    MangledFieldName = "data_tag" } 
> +	
> +	->

Fix the layout.

> +		mlds_output_bracketed_rval(PtrRval),
> +		io__write_string(".")
> +	;
> +		io__write_string("(("),
> +		mlds_output_type(CtorType),
> +		io__write_string(") "),
> +		mlds_output_bracketed_rval(PtrRval),  % the actual variable
> +		io__write_string(").")

A comment explaining why the cast is needed here might be helpful.

> +mlds_output_rval(mkword(Tag, Rval)) -->
> +	io__write_string("(MR_Word) MR_mkword("),
> +	mlds_output_tag(Tag),
> +	io__write_string(", "),
> +	mlds_output_rval(Rval),
> +	io__write_string(")").

I think that is going to be wrong for the Java back-end;
it might be better to call unexpected/2 here.

> +mlds_output_rval(mem_addr(Lval)) -->
> +	% XXX the MLDS code generator should probably not generate these
> +	% io__write_string("&"),
> +	mlds_output_lval(Lval).

I suggest you change that to output `/* & */' and then see if any
actually get generated (and if so, figure out why).

> +:- pred mlds_output_unop(mlds__unary_op, mlds__rval, io__state, io__state).
> +:- mode mlds_output_unop(in, in, di, uo) is det.
> +	
> +mlds_output_unop(cast(Type), Exprn) -->
> +	mlds_output_cast_rval(Type, Exprn).
> +mlds_output_unop(box(Type), Exprn) -->
> +	mlds_output_boxed_rval(Type, Exprn).
> +mlds_output_unop(unbox(Type), Exprn) -->
> +	mlds_output_unboxed_rval(Type, Exprn).
> +mlds_output_unop(std_unop(Unop), Exprn) -->
> +	mlds_output_std_unop(Unop, Exprn).
> +
> +:- pred mlds_output_cast_rval(mlds__type, mlds__rval, io__state, io__state).
> +:- mode mlds_output_cast_rval(in, in, di, uo) is det.
> +% XXX Putting these casts in seems pointless.	
> +mlds_output_cast_rval(_Type, Exprn) -->
> +	%io__write_string("("),
> +	%mlds_output_type(Type),
> +	%io__write_string(") "),
> +	mlds_output_rval(Exprn).

Why is that commented out?

I think that if the MLDS code generator generates casts, then the Java
back-end should respect that.  If the result is too many unnecessary
casts, then the right fix is probably to change the MLDS code generator
so that it generates less casts, rather than ignoring the casts that
it does generate.  Some of them may be needed.

> +:- pred mlds_output_std_unop(builtin_ops__unary_op, mlds__rval,
> +		io__state, io__state).
> +:- mode mlds_output_std_unop(in, in, di, uo) is det.
> +	
> +mlds_output_std_unop(UnaryOp, Exprn) -->
> +	{ c_util__unary_prefix_op(UnaryOp, UnaryOpString) },
> +	io__write_string(UnaryOpString),
> +	io__write_string("("),
> +	( { UnaryOp = tag } ->
> +		% XXX This is probably not right in the Java backend.
> +		% The MR_tag macro requires its argument to be of type 
> +		% `MR_Word'.
> +		% XXX should we put this cast inside the definition of MR_tag?
> +		io__write_string("(MR_Word) ")
> +	;
> +		[]
> +	),
> +	mlds_output_rval(Exprn),
> +	io__write_string(")").

I suggest you delete the test for `tag' and the cast to `MR_Word'.

Calling c_util__unary_prefix_op here is not ideal, since
Java is not C.  If you're going to do that, then you should
modify the documentation for c_util__unary_prefix_op to say
that what it returns should be valid in Java too.

> +:- pred mlds_output_binop(binary_op, mlds__rval, mlds__rval,
> +			io__state, io__state).
> +:- mode mlds_output_binop(in, in, in, di, uo) is det.
> +	
> +mlds_output_binop(Op, X, Y) -->
> +	(
...
> +		{ c_util__string_compare_op(Op, OpStr) }
> +	->
> +		mlds_output_rval(X),
> +		io__write_string(".equals("),
> +		mlds_output_rval(Y),
> +		io__write_string(")"),
> +		io__write_string(" "),
> +		io__write_string(OpStr),
> +		io__write_string(" "),
> +		io__write_string("true")

That looks wrong to me.  It's OK for `==' and `!=',
but it looks wrong for `<', `<=', etc.

> +	% Output an Rval, if the Rval represents an enumeration object
> +	% put in the ".value" so we can access its value.

That's not a grammatically correct sentence.

> +:- pred mlds_output_binary_op(binary_op, io__state, io__state).
> +:- mode mlds_output_binary_op(in, di, uo) is det.
> +
> +mlds_output_binary_op(Op) -->
> +	( { c_util__binary_infix_op(Op, OpStr) } ->
> +		io__write_string(OpStr)


See comment above for c_util__unary_prefix_op.

> +mlds_output_rval_const(string_const(String)) -->
> +	io__write_string(""""),
> +	c_util__output_quoted_string(String),
> +	io__write_string("""").

Likewise here.

> +mlds_output_rval_const(multi_string_const(Length, String)) -->
> +	io__write_string(""""),
> +	c_util__output_quoted_multi_string(Length, String),
> +	io__write_string("""").

And here.

> +%-----------------------------------------------------------------------------%
> +%
> +% Miscellaneous stuff to handle indentation and generation of
> +% source context annotations.  (XXX This can probably be simplified
> +% since Java doens't have an equivalent of #line directives.)

s/doens't/doesn't/

OK, that completes this review.  Apart from the issues raised above
and in my earlier emails, this looks good.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "I have always known that the pursuit
                                    |  of excellence is a lethal habit"
WWW: <http://www.cs.mu.oz.au/~fjh>  |     -- 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
--------------------------------------------------------------------------