[m-dev.] for review: changes to MLDS

[Fergus Henderson]

On 31-Jan-2001, Julien Fischer <juliensf at students.cs.mu.oz.au> wrote:
> 
> +++ compiler/ml_code_util.m	2001/01/31 01:41:58
...
> +	% Generate declaration flags for a local variable
> +	%
> +	%
> +:- func ml_gen_local_var_decl_flags = mlds__decl_flags.

Delete one of those two blank comment lines.

>  ml_initial_cont(OutputVarLvals0, OutputVarTypes0, Cont) -->
> -	ml_qualify_var("cont", ContLval),
> -	ml_qualify_var("cont_env_ptr", ContEnvLval),
>  	{ ml_skip_dummy_argument_types(OutputVarTypes0, OutputVarLvals0,
>  		OutputVarTypes, OutputVarLvals) },
>  	list__map_foldl(ml_gen_type, OutputVarTypes, MLDS_OutputVarTypes),
> +	ml_gen_var_lval("cont", mlds__cont_type(MLDS_OutputVarTypes), ContLval),
> +	ml_gen_var_lval("cont_env_ptr", mlds__generic_env_ptr_type,
> +		ContEnvLval),
>  	{ Cont = success_cont(lval(ContLval), lval(ContEnvLval),
>  		MLDS_OutputVarTypes, OutputVarLvals) }.

I think that should be `mlds__cont_type([])' when the
`--nondet-copy-out' option is not enabled.

> +++ compiler/ml_type_gen.m	2001/01/31 01:41:58
...
> +	% Return the declaration flags appropriate for a member of a class
> +	% that was transformed from a special predicate.  These differ 
> +	% from normal members in that their finality is final.
> +	%
> +:- func ml_gen_special_member_decl_flags = mlds__decl_flags.

I suggest s/final/`final'/

> +++ compiler/ml_unify_gen.m	2001/01/31 01:41:58
> @@ -307,8 +307,11 @@
>  		% If this argument is something that would normally be allocated
>  		% on the heap, just generate a reference to the static constant
>  		% that we must have already generated for it.
> +		% XXX This is probably wrong when `--high-level-data' is 
> +		%     enabled.
>  		%
> -		ml_gen_static_const_addr(Var, ConstAddrRval),
> +		{ ConstType = mlds__array_type(mlds__generic_type) },	
> +		ml_gen_static_const_addr(Var, ConstType, ConstAddrRval),

I suggest elaborating the XXX comment a little:
s/This/Using mlds__array_type(mlds__generic_type)/

> @@ -1012,6 +1018,8 @@
>  
>  		%
>  		% Generate a local static constant for this term.
> +		% XXX This is probably wrong when `--high-level-data'
> +		%     is enabled.
>  		%
>  		ml_gen_static_const_name(Var, ConstName),
>  		{ ConstType = mlds__array_type(mlds__generic_type) },

Likewise here.

> Index: compiler/ml_util.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/ml_util.m,v
> retrieving revision 1.5
> diff -u -r1.5 ml_util.m
> --- compiler/ml_util.m	2001/01/11 14:25:39	1.5
> +++ compiler/ml_util.m	2001/01/31 01:41:58
> @@ -68,8 +68,8 @@
>  :- pred defn_is_public(mlds__defn).
>  :- mode defn_is_public(in) is semidet.
>  
> -%-----------------------------------------------------------------------------%
>  
> +%-----------------------------------------------------------------------------%
>  :- implementation.
>  
>  :- import_module rtti.

Don't commit that change to ml_util.m.

> Index: compiler/mlds.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mlds.m,v
> retrieving revision 1.45
> diff -u -r1.45 mlds.m
> --- compiler/mlds.m	2001/01/17 17:37:17	1.45
> +++ compiler/mlds.m	2001/01/31 01:41:59
> @@ -547,8 +547,15 @@
>  
>  	;	mlds__pseudo_type_info_type
>  	
> -	;	mlds__rtti_type(rtti_name).
> +	;	mlds__rtti_type(rtti_name)
>  
> +		% A type used internally by the ML code generator to 
> +		% mark variables whose type is yet to be generated.  This
> +		% occurs once in ml_code_util.m where env_ptr's are created,
> +		% but their type remains unknown until the ml_elim_nested.m
> +		% pass.  
> +	;	mlds__unknown_type.

The comment here is misleading, because it suggests that the ml_elim_nested.m
will replace all occurrences of mlds__unknown_type with the right value.
But that pass doesn't do that.  Instead it leaves them as mlds__unknown_type.

> Index: compiler/mlds_to_c.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mlds_to_c.m,v
> retrieving revision 1.78
> diff -u -r1.78 mlds_to_c.m
> --- compiler/mlds_to_c.m	2001/01/29 06:47:16	1.78
> +++ compiler/mlds_to_c.m	2001/01/31 01:41:59
> @@ -632,7 +632,8 @@
>  	io__write_string("MR_Word").
>  mlds_output_pragma_export_type(prefix, mlds__rtti_type(_)) -->
>  	io__write_string("MR_Word").
> -	
> +mlds_output_pragma_export_type(prefix, mlds__unknown_type) -->
> +	{ error("mlds_to_c.m: prefix has unknown type") }. 

The error message here is misleading;
the problem is not that the prefix has an unknown type,
it is that you're trying to output the prefix of an unknown type.

Also, it would be better to use unexpected/2.
So I suggest

	{ unexpected(this_file,
		"mlds_output_pragma_export_type: unknown_type") }.

where `this_file' is defined appropriately.

> +mlds_output_type_prefix(mlds__unknown_type) -->
> +	{ error("mlds_to_c.m: prefix has unknown type") }.

Likewise here.

> +mlds_output_type_suffix(mlds__unknown_type, _) -->
> +	{ error("mlds_to_c.m: suffix has unknown type") }.

And here.

> Index: compiler/mlds_to_il.m
> @@ -1813,6 +1814,8 @@
> +mlds_type_to_ilds_type(mlds__unknown_type) = _ :-
> +	 error("mlds_to_il.m: unknown type").	

Use unexpected/2.

Otherwise that looks good.  I'd like to see a relative diff when
you've addressed those review comments.

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