[m-rev.] for review: Modifications to the Java back-end

[Fergus Henderson]

On 22-Jan-2002, Michael Wybrow <mjwybrow at students.cs.mu.oz.au> wrote:
> +generate_call_method(CodeAddr, MethodDefn) :-
...
> +	ReturnDecFlags = init_decl_flags(private, one_copy, non_virtual,
> +			final, modifiable, concrete),

That's wrong -- it shouldn't be "private" or "one_copy".

Use ml_gen_local_var_decl_flags.

> +	ReturnEntityDefn = mlds__data(ReturnVarType, no_initializer, no), 

It might be a good idea to write that as

	GCTraceCode = no,
	ReturnEntityDefn = mlds__data(ReturnVarType, no_initializer,
		GCTraceCode), 

and to add a comment explaining why it's OK to set GCTraceCode = no here.

> +	%
> +	% Create a return statement that returns the result of the call to the
> +	% original method, boxed as a java.lang.Object.
> +	% 
> +	ReturnRval = unop(box(mlds__generic_type), lval(ReturnLval)),

The argument of box/1 should be the *original* type of the entity being boxed
(i.e. `MethodRetType' in this case, I think), not the type of the boxed result.

> +	Block = block(MethodDefns, [CallStatement|[ReturnStatement]]),

Write that as `[CallStatement, ReturnStatement]'.

> +:- pred generate_call_method_args(list(mlds__type), mlds__var, int, 
> +		list(mlds__rval), list(mlds__rval)).
> +:- mode generate_call_method_args(in, in, in, in, out) is det.
> +
> +generate_call_method_args([], _, _, Args, Args).
> +generate_call_method_args([_Type|Types], Variable, Counter, Args0, Args) :-
> +	ArrayRval = mlds__lval(mlds__var(Variable, mlds__native_int_type)),

Using mlds__native_int_type here is wrong.
The type that should be used is the type of the array
(`mlds__array(mlds__generic_type)', I think).

> +	Rval = binop(array_index(elem_type_generic), ArrayRval, IndexRval),
> +	BoxedRval = unop(unbox(mlds__generic_type), Rval),

s/BoxedRval/UnboxedRval/g

The argument of unbox/1 should the final unboxed type,
i.e. the variable `_Type' in this case.

> +:- pred symbol_name_to_string(sym_name, string, string).
> +:- mode symbol_name_to_string(in, in, out) is det.
> +
> +symbol_name_to_string(unqualified(SymName), SymNameStr0, SymNameStr) :-
> +	SymNameStr = SymNameStr0 ++ SymName.
> +symbol_name_to_string(qualified(Qualifier, SymName), SymNameStr0, SymNameStr) :-
> +	symbol_name_to_string(Qualifier, SymNameStr0, SymNameStr1),
> +	SymNameStr = SymNameStr1 ++ "__" ++ SymName.

It would be better to make that a function rather than a predicate.

> @@ -610,38 +1097,49 @@
...
> +	% Discriminated union which allows us to pass down the class name if 
> +	% a definition is a constructor, this is needed since the class name
> +	% is not available for a constructor in the mlds.

s/,/;/

> @@ -675,6 +1168,12 @@
>  	output_implements_list(Implements),
>  	io__write_string(" {\n"),
>  	output_class_body(Indent + 1, Kind, Name, AllMembers, ModuleName),
> +	io__nl,
> +	( { Ctors = [] } ->
> +		[]		% No constructors.
> +	;
> +		output_defns(Indent + 1, ModuleName, cname(UnqualName),	Ctors)
> +	),

Do you need to special-case the Ctors = [] case here?
Why not just call output_defns unconditionally?

> @@ -1556,21 +2075,94 @@
>  		%
>  		io__write_string("java.lang.Object [] result = ")
>  	),
> -	( { MaybeObject = yes(Object) } ->
> -		output_bracketed_rval(Object),
> -		io__write_string(".")
> +	( { FuncRval = lval(var(_, func_type(_))) } -> 

That's not the right way to test if it is a call via a function pointer.
For example, the function pointer might be a field of some data structure,
rather than a variable.

Instead, you should test for

	( { FuncRval \= const(code_addr_const(_)) } ->

and then it would be nicer to swap the "then" and the "else" parts
so you can test for "=" rather than "\=".

> +		( { MaybeObject = yes(Object) } ->
> +			output_bracketed_rval(Object),
> +			io__write_string(".")
> +		;
> +			[]
> +		),

These five lines seem to be the same for both the "then" and then "else"
parts, in which case they should be extracted out

> +		%
> +		% Here we do downcasting, as a call will always return
> +		% something of type java.lang.Object
> +		%
> +		% XXX This is a hack, I can't see any way to do this
> +		%     downcasting nicely, as it needs to effectively be
> +		%     wrapped around the method call itself, so it acts
> +		%     before this predicate's solution to multiple return
> +		%     values, see above.

Maybe the best way to do it is to write a higher-order Mercury
predicate to do an unbox operation; this would take as an argument
a closure that prints out the Java expression which is being unboxed.

However, don't worry about that until after the rest of this is committed.

> +		%
> +		% This is a function pointer call. It may look strange that we
> +		% use "output_nonaddressed_rval" with the call, this is
> +		% because the "AddOf__whatever" will have been stored as a 
> +		% MethodPtr variable - "FuncRval" will be that variable, so we
> +		% want to output it normally.
> +		%
> +		output_nonaddressed_rval(FuncRval),

s/AddOf/AddrOf/

But actually this comment is bogus -- you can (and probably should) just use
`output_bracketed_rval' here.  The rval won't be a code_addr_const
anyway, because the condition of the if-then-else that this code is
inside already checked that.

...
> +:- pred output_args_as_array(list(mlds__rval), io__state, io__state).
> +:- mode output_args_as_array(in, di, uo) is det.
> +
> +output_args_as_array(CallArgs) -->
> +	io__write_string("new java.lang.Object[] { "),
> +	output_boxed_args(CallArgs, mlds__generic_type),

Again you've got the type for the argument of box/1 wrong.
You need to pass in the list of the argument types to this routine
and use the appropriate type for each

> -output_atomic_stmt(Indent, _FuncInfo, NewObject, Context) -->
> +output_atomic_stmt(Indent, _FuncInfo, NewObject, _Context) -->
>  	{ NewObject = new_object(Target, _MaybeTag, _HasSecTag, Type,
>  		_MaybeSize, MaybeCtorName, Args, ArgTypes) },
>  	
>  	indent_line(Indent),
> -	io__write_string("{\n"),
> -	indent_line(Context, Indent + 1),

The `{' is needed, I'm pretty sure,
in case this statement is the body of an if-then-else.

> -	io__write_string("();\n"),
> -	output_init_args(Args, ArgTypes, CtorDefn, Context, 0, Target, 0,
> -		Indent + 1),
> -	indent_line(Context, Indent),
> -	io__write_string("}\n").
> +	( { Type = mlds__func_type(_FuncParams);
> +	    Type =mlds__mercury_type(_Type, pred_type, _) } 
> +	->

The condition of the if-then-else should be indented if it doesn't fit
on a single line, and the ";" for a disjunction should be at the start
of a line.  Also s/=m/= m/

> +		%
> +		% The new object will be an array of java.lang.Object, we need
> +		% to initialise it using array literals syntax.

Isn't this true only for closures (mercury_type(_, pred_type, _)),
not for function pointers?  Function pointers should never be
used in new_object() instructions.

Also s/, we/, so we/

> @@ -1937,8 +2500,25 @@
> +output_lval(field(_MaybeTag, Rval, offset(OffsetRval), FieldType,
> +		_ClassType)) -->
> +	(
> +		{ FieldType = mlds__generic_type
> +		; FieldType = mlds__mercury_type(term__variable(_), _, _)
> +		}
> +	->
> +		io__write_string("(")
> +	;
> +		% The field type for field(_, _, offset(_), _, _) lvals
> +		% must be something that maps to MR_Box.
> +		{ error("unexpected field type") }
> +	),
> +	output_rval(Rval),
> +	io__write_string("["),
> +	output_rval(OffsetRval),
> +	io__write_string("]))").

Move the call to io__write_string("(") to after the if-then-else
(change the "then" part of the if-then-else to just "[]").

> +:- pred output_nonaddressed_rval(mlds__rval, io__state, io__state).
> +:- mode output_nonaddressed_rval(in, di, uo) is det.

There should be a comment here explaining what you mean by a "nonaddressed"
rval.

> +output_nonaddressed_rval(Rval) -->
> +	(
> +		{ Rval = mlds__const(Const),
> +		Const = mlds__code_addr_const(CodeAddr) }
> +	->
> +		io__write_char('('),
> +		{ IsCall = yes },
> +		mlds_output_code_addr(CodeAddr, IsCall),
> +		io__write_char(')')

I'm pretty sure the '(' and ')' aren't needed here, are they?

> Index: java_util.m
...
> +java_util__unary_prefix_op(unmktag, 		"/* unmaktag */ ").

s/mak/mk/

> +java_util__unary_prefix_op(unmkbody,		"/* unmakbody */ ").

Likewise.

Otherwise this change looks fine.

Please post both a relative diff (e.g. using `interdiff -h')
and a new full diff when you've addressed these review comments.

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