[m-dev.] for review: polymorphic ground insts [3/3]

[Fergus Henderson]

On 09-Feb-2000, David Overton <dmo at ender.cs.mu.oz.au> wrote:
> Index: compiler/modes.m
> @@ -607,11 +607,27 @@
>  	{ pred_info_procedures(PredInfo0, Procs0) },
>  	{ map__keys(Procs0, ProcIds) },
>  	( { WhatToCheck = check_modes } ->
> -		( { ProcIds = [] } ->
> +		(
> +			{ ProcIds = [] }
> +		->
>  			maybe_report_error_no_modes(PredId, PredInfo0,
>  					ModuleInfo0),
>  			{ NumErrors0 = 0 }
>  		;
> +			{ module_info_get_special_pred_map(ModuleInfo0,
> +				SpecialPredMap) },
> +			{ map__member(SpecialPredMap, unify - _, PredId) }
> +		->
> +			% Don't check for indistinguishable modes in unification
> +			% predicates.

Calling map__member on the special_pred_map is likely to be quite
inefficient.  If you want to check whether a predicate is a unification
predicate, it would be more efficient to just check the name and arity,
e.g. via

		special_pred_name_arity(unify, _, PredName, Arity),
		pred_info_name(PredInfo0, PredName),
		pred_info_arity(PredInfo0, PredArity)

> +++ compiler/prog_data.m	2000/02/03 03:44:26
> @@ -698,7 +698,7 @@
>  	;	abstract_inst(sym_name, list(inst_param)).
>  
>  	% probably inst parameters should be variables not terms
> -:- type inst_param	==	inst_term.
> +:- type inst_param	==	inst_var.

You can delete that comment now.

It would probably be best to do a global s/inst_param/inst_var/g
and then just delete the inst_param type.

> +++ compiler/special_pred.m	2000/02/07 04:02:01
> @@ -67,6 +67,11 @@
>  	% mode num for special procs is always 0 (the first mode)
>  special_pred_mode_num(_, 0).
>  
> +	% XXX If the type has only one value, the determinism should be `det'.
> +	% However, this predicate is called by make_hlds before all the type
> +	% information is available, so we can't check that here.
> +	% There is a pass over the unify preds at the end of make_hlds to
> +	% fix up the determinism.
>  special_pred_info(unify, Type, "__Unify__", [Type, Type], [In, In], semidet) :-
>  	in_mode(In).

I don't think that comment deserves an XXX.
An XXX should mark something that is or at least might be wrong,
and which thus deserves revisiting.  But in this case the comment
in the last sentence says that the problem is already solved,
so it doesn't need revisiting, right?

But that information about what special_pred_info returns is needed
to understand how to use that predicate, so a comment like that one
should go in the interface section of the module.

> Index: compiler/type_util.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/type_util.m,v
> retrieving revision 1.79
> diff -u -r1.79 type_util.m
> --- compiler/type_util.m	2000/02/08 06:59:28	1.79
> +++ compiler/type_util.m	2000/02/09 00:19:41
> @@ -171,6 +171,11 @@
>  :- pred type_util__get_cons_id_arg_types(module_info::in, (type)::in,
>  		cons_id::in, list(type)::out) is det.
>  
> +	% The same as type_util__get_cons_id_arg_types except that the
> +	% cons_id is output non-deterministically.
> +:- pred type_util__cons_id_arg_types(module_info::in, (type)::in,
> +		cons_id::out, list(type)::out) is nondet.

Why not just call type_util__get_cons_id_arg_types
and then list__member?

>  	% Given a type and a cons_id, look up the definitions of that
>  	% type and constructor. Aborts if the cons_id is not user-defined.
>  :- pred type_util__get_type_and_cons_defn(module_info, (type), cons_id,
> @@ -664,20 +669,39 @@
>  			ConsId, TypeDefn, ConsDefn),
>  		ConsDefn = hlds_cons_defn(ExistQVars0, _Constraints0,
>  				ArgTypes0, _, _),
> -		ArgTypes0 \= []
> +		ArgTypes0 \= [],
> +
> +		% XXX handle ExistQVars
> +		ExistQVars0 = []
>  	->
>  		hlds_data__get_type_defn_tparams(TypeDefn, TypeDefnParams),
>  		term__term_list_to_var_list(TypeDefnParams, TypeDefnVars),
>  
> -		% XXX handle ExistQVars
> -		require(unify(ExistQVars0, []),
> -	"type_util__get_cons_id_arg_types: existentially typed cons_id"),
> -
>  		map__from_corresponding_lists(TypeDefnVars, TypeArgs, TSubst),
>  		term__apply_substitution_to_list(ArgTypes0, TSubst, ArgTypes)
>  	;
>  		ArgTypes = []
>  	).

That change is not documented in your log message.
The change breaks the documentation for that predicate.
And I don't understand the rationale for it.

> Index: library/array.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/array.m,v
> retrieving revision 1.67
> diff -u -r1.67 array.m
> --- library/array.m	2000/01/19 09:45:16	1.67
> +++ library/array.m	2000/02/04 02:15:56
> @@ -93,30 +93,30 @@
>  	% Note: in this implementation, the lower bound is always zero.
>  :- pred array__min(array(_T), int).
>  :- mode array__min(array_ui, out) is det.
> -:- mode array__min(in, out) is det.
> +%:- mode array__min(in, out) is det.
>  
>  	% array__max returns the upper bound of the array.
>  :- pred array__max(array(_T), int).
>  :- mode array__max(array_ui, out) is det.
> -:- mode array__max(in, out) is det.
> +%:- mode array__max(in, out) is det.

You should explain why all those modes are commented out.

In fact, I don't understand why those modes need to be commented out.
Why is the bound_inst_list_is_complete_for_type predicate
succeeding for the builtin type `array'?

If they do need to be commented out, this might cause problems
for similar user code using arrays.  It might be a better idea
to simply disable the check for indistinguisable modes,
at least until `ui' modes are properly supported.


Anyway, that completes my review.
Apart from the issues I've raised so far,
and the couple of things that you yourself listed
as still do be done, this looks good.
This change will need another round of reviewing, IMHO.
I'd like to see a relative diff next time.

Thanks,
	Fergus.

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