[m-dev.] for review: reordering for existential types [2]

[David Glen JEFFERY]

Hi,

The review continues... there's a whole bunch of diffs for me to review here.
I've had a look through most of them already and they look fine, but I will
reply to each in turn with any minor niggles.

On 11-Jun-1999, Fergus Henderson <fjh at cs.mu.OZ.AU> wrote:
> [continued from part 1]
> 
> Index: compiler/polymorphism.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/polymorphism.m,v
> retrieving revision 1.163
> diff -u -r1.163 polymorphism.m
> --- polymorphism.m	1999/04/23 01:02:57	1.163
> +++ polymorphism.m	1999/06/09 19:55:09
> @@ -427,17 +454,17 @@
>  
>  polymorphism__process_preds([], ModuleInfo, ModuleInfo) --> [].
>  polymorphism__process_preds([PredId | PredIds], ModuleInfo0, ModuleInfo) -->
> -	polymorphism__process_pred(PredId, ModuleInfo0, ModuleInfo1),
> +	polymorphism__maybe_process_pred(PredId, ModuleInfo0, ModuleInfo1),
>  	polymorphism__process_preds(PredIds, ModuleInfo1, ModuleInfo).
>  
> -:- pred polymorphism__process_pred(pred_id, module_info, module_info,
> +:- pred polymorphism__maybe_process_pred(pred_id, module_info, module_info,
>  			io__state, io__state).
> -:- mode polymorphism__process_pred(in, in, out, di, uo) is det.
> +:- mode polymorphism__maybe_process_pred(in, in, out, di, uo) is det.

Why has the `maybe' been introduced here? It is hard to tell from the diff.

> -polymorphism__process_pred(PredId, ModuleInfo0, ModuleInfo, IO0, IO) :-
> -	module_info_pred_info(ModuleInfo0, PredId, PredInfo),
> +polymorphism__maybe_process_pred(PredId, ModuleInfo0, ModuleInfo) -->
> +	{ module_info_pred_info(ModuleInfo0, PredId, PredInfo) },
>  	(
> -		(
> +		{
>  			% Leave Aditi aggregates alone, since
>  			% calls to them must be monomorphic. This avoids
>  			% unnecessarily creating type_infos in Aditi code,



> +polymorphism__process_pred(PredId, ModuleInfo0, ModuleInfo) -->
> +	{ module_info_pred_info(ModuleInfo0, PredId, PredInfo0) },
> +
> +	write_pred_progress_message("% Transforming polymorphism for ",
> +					PredId, ModuleInfo0),
> +

Is that meant to be there (in the checked-in version)? (...or does the
write_pred_progress_message stuff only get output with -V?)

> +polymorphism__process_proc(ProcId, ProcInfo0, PredInfo, ClausesInfo,
> +			ExtraArgModes, ProcInfo) :-
> +	%
> +	% copy all the information from the clauses_info into the proc_info
> +	%
> +	(
> +		( pred_info_is_imported(PredInfo)
> +		; pred_info_is_pseudo_imported(PredInfo),
> +		  hlds_pred__in_in_unification_proc_id(ProcId)
>  		)
> +	->
> +		% XXX is this right?
> +		ProcInfo1 = ProcInfo0

Looks right to me... but it should be easy enough to test.

>  % XXX the following code ought to be rewritten to handle
>  % existential/universal type_infos and type_class_infos
>  % in a more consistent manner.

That comment can stay there. ;-)

>  polymorphism__assign_var_2(Var1, Var2, Goal) :-
> +	term__context_init(Context),
> +	create_atomic_unification(Var1, var(Var2), Context, explicit,
> +		[], Goal).
>  
> -	% Doing just this wouldn't work, because we also need to fill in
> -	% the mode and determinism info:
> -	%	term__context_init(Context),
> -	%	create_atomic_unification(Var1, var(Var2), Context, explicit,
> -	% 		[], Goal),
> -

Isn't that lovely? ;-)

> +polymorphism__process_unify_functor(X0, ConsId0, ArgVars0, Mode0,
> +		Unification0, UnifyContext, GoalInfo0, Goal,
> +		PolyInfo0, PolyInfo) :-

>  	(
> -		{ polymorphism__no_type_info_builtin(PredModule,
> -			PredName, PredArity)  }

Where does this case get handled now? (I see a call to
polymorphism__no_type_info_builtin in part of the diff that I've deleted,
but I can't tell from the context what it is doing...).

> +		%
> +		% now process it
> +		%
> +		%polymorphism__process_goal_expr(HOCall, GoalInfo0, Goal,
> +		%	PolyInfo0, PolyInfo)

Why is this commented out/why is this comment here?

> +	% If it turns out that the predicate was non-polymorphic,
> +	% lambda.m will (I hope) turn the lambda expression
> +	% back into a higher-order pred constant again.

That sounds like the right way to handle it, but hoping probably isn't the
best thing to do...

> +			error("modecheck_unification: list__drop failed")

modecheck_unification isn't the predname any more.

> +			error("Sorry, not implemented: determinism inference for higher-order predicate terms")

Ouch!

> +% this is duplicated in modecheck_unify.m
> +:- pred make_fresh_vars(list(type), prog_varset, map(prog_var, type),
> +			list(prog_var), prog_varset, map(prog_var, type)).
> +:- mode make_fresh_vars(in, in, in, out, out, out) is det.

At the least you should XXX that. (Have I just verbed a new noun?)

It could probably go in <something>_util.m, though.

[ skipping quantification.m, it is in the next diff ].

[ I've basically skipped over simplify.m as I'm not really familiar with the
code in there... it looks like Simon has looked over the diff, though, so
it should be fine ].



dgj
-- 
David Jeffery (dgj at cs.mu.oz.au) | If your thesis is utterly vacuous
PhD student,                    | Use first-order predicate calculus.
Dept. of Comp. Sci. & Soft. Eng.|     With sufficient formality
The University of Melbourne     |     The sheerist banality
Australia                       | Will be hailed by the critics: "Miraculous!"
                                |     -- Anon.
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------