[m-rev.] For review: solver-types

[Zoltan Somogyi]

On 13-Aug-2004, Ralph Becket <rafe at cs.mu.OZ.AU> wrote:
> Index: compiler/equiv_type.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/equiv_type.m,v
> retrieving revision 1.39
> diff -u -r1.39 equiv_type.m
> --- compiler/equiv_type.m	14 Jun 2004 04:49:07 -0000	1.39
> +++ compiler/equiv_type.m	13 Aug 2004 00:30:07 -0000
> @@ -399,9 +399,25 @@
>  		_, ContainsCirc, !VarSet, !Info).
>  
>  equiv_type__replace_in_type_defn(EqvMap, _,
> -		du_type(TBody0, IsSolverType, EqPred),
> -		du_type(TBody, IsSolverType, EqPred), no, !VarSet, !Info) :-
> +		du_type(TBody0, SpecialTypeDetails),
> +		du_type(TBody, SpecialTypeDetails), no, !VarSet, !Info) :-
>  	equiv_type__replace_in_ctors(EqvMap, TBody0, TBody, !VarSet, !Info).
> +
> +equiv_type__replace_in_type_defn(EqvMap, TypeCtor,
> +		solver_type(SpecialTypeDetails0),
> +		solver_type(SpecialTypeDetails),
> +		ContainsCirc, !VarSet, !Info) :-
> +	SpecialTypeDetails0 ^ solver_type_details = yes(SolverTypeDetails0),
> +	RepresentationType0 = SolverTypeDetails0 ^ representation_type,
> +	equiv_type__replace_in_type_2(EqvMap, [TypeCtor], 
> +		RepresentationType0, RepresentationType,
> +		_, ContainsCirc, !VarSet, !Info),
> +	SolverTypeDetails =
> +		SolverTypeDetails0 ^ representation_type :=
> +			RepresentationType,
> +	SpecialTypeDetails =
> +		SpecialTypeDetails0 ^ solver_type_details :=
> +			yes(SolverTypeDetails).

It would be better not to use field access functions here. If a new
argument is added, you want this code to generate an error, forcing the
programmer to think about substitutions in the new field.

> --- compiler/equiv_type_hlds.m	14 Jun 2004 04:16:02 -0000	1.6
> +++ compiler/equiv_type_hlds.m	13 Aug 2004 02:12:42 -0000
> @@ -129,11 +129,28 @@
>  			TVarSet0, TVarSet, EquivTypeInfo0, EquivTypeInfo),
>  		Body = eqv_type(Type)
>  	;
> -		Body0 = foreign_type(_, _),
> +		Body0 = foreign_type(_),
>  		EquivTypeInfo = EquivTypeInfo0,
>  		Body = Body0,
>  		TVarSet = TVarSet0
>  	;
> +		Body0 = solver_type(SolverTypeDetails0, SpecialTypeDetails0),
> +		RepnType0 = SolverTypeDetails0 ^ representation_type,
> +		equiv_type__replace_in_type(EqvMap, RepnType0, RepnType, _,
> +			TVarSet0, TVarSet, EquivTypeInfo0, EquivTypeInfo),
> +		SolverTypeDetails =
> +			SolverTypeDetails0 ^ representation_type := RepnType,
> +		( 
> +			SpecialTypeDetails1 =
> +				SpecialTypeDetails0 ^ solver_type_details :=
> +					yes(SolverTypeDetails)
> +		->
> +			SpecialTypeDetails = SpecialTypeDetails1
> +		;
> +			SpecialTypeDetails = SpecialTypeDetails0
> +		),
> +		Body = solver_type(SolverTypeDetails, SpecialTypeDetails)
> +	;
>  		Body0 = abstract_type(_),
>  		EquivTypeInfo = EquivTypeInfo0,
>  		Body = Body0,

Same here.

> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/foreign.m,v
> retrieving revision 1.41
> diff -u -r1.41 foreign.m
> --- compiler/foreign.m	28 Jun 2004 04:49:42 -0000	1.41
> +++ compiler/foreign.m	14 Jul 2004 06:44:17 -0000
> @@ -24,7 +24,7 @@
>  :- import_module libs__globals.
>  :- import_module parse_tree__prog_data.
>  
> -:- import_module bool, std_util, list, string, term.
> +:- import_module bool, list, std_util, string, term.
>  
>  :- type foreign_decl_info 		== list(foreign_decl_code).
>  					% in reverse order
> @@ -94,14 +94,14 @@
>  	% Does the implementation of the given foreign type body on
>  	% the current backend use a user-defined comparison predicate.
>  :- func foreign_type_body_has_user_defined_eq_comp_pred(module_info,
> -	foreign_type_body) = unify_compare is semidet.
> +	foreign_type_body) = special_type_details is semidet.

>  	% Find the current target backend from the module_info, and given
>  	% a foreign_type_body, return the name of the foreign language type
>  	% the identity of any user-defined unify/compare predicates, and the
>  	% assertions applicable to that backend.
>  :- pred foreign_type_body_to_exported_type(module_info::in,
> -	foreign_type_body::in, sym_name::out, maybe(unify_compare)::out,
> +	foreign_type_body::in, sym_name::out, special_type_details::out,
>  	list(foreign_type_assertion)::out) is det.

These changes should be undone. Your data structures are still too loose.

> Index: compiler/hlds_data.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/hlds_data.m,v
> retrieving revision 1.87
> diff -u -r1.87 hlds_data.m
> --- compiler/hlds_data.m	14 Jun 2004 04:16:06 -0000	1.87
> +++ compiler/hlds_data.m	12 Aug 2004 06:52:07 -0000
> @@ -142,24 +142,29 @@
>  					% is this type an enumeration?
>  			du_type_is_enum		:: bool,
>  
> -					% user-defined equality and
> -					% comparison preds
> -			du_type_usereq		:: maybe(unify_compare),
> +					% Optional user-defined equality and
> +					% comparison preds, but no
> +					% solver_type_details.
> +			du_type_special_type_details :: special_type_details,

This field shouldn't be of type special_type_details. You should use a
different type that doesn't have a slot for solver_type_details, e.g.
(a variant of) the existing type maybe(unify_compare).

> @@ -171,10 +176,13 @@
>  
>  :- type foreign_type_lang_body(T) == maybe(foreign_type_lang_data(T)).
>  
> +	% Foreign types may have user-defined equality and comparison
> +	% preds, but not solver_type_details.
> +	%
>  :- type foreign_type_lang_data(T)
>  	--->	foreign_type_lang_data(
>  			T,
> -			maybe(unify_compare),
> +			special_type_details,
>  			list(foreign_type_assertion)
>  		).

Same here. The comment is in direct contradiction to the change.

If a type is a solver type, then hlds_type_body should be bound to solver_type,
not du_type or foreign_type. Because of this, the du_type and
foreign_type function symbols should have no descendant fields of type
solver_type_details. If they do, as in your design (since
special_type_details includes a field of type maybe(solver_type_details)),
then code processing du_types and foreign_types must choose between two
bad choices: either it has lots of error checking code that wouldn't be
needed with a better data structure design, or it ignores data values that
violate the data structure's invariants.

If at all possible, it is always better to design types so that all
values of the type are meaningful. As far as I can tell, leaving the
du_type_usereq field of du_type and the second field of
foreign_type_lang_data of type maybe(unify_compare) ensures this.
It also leads to many fewer changes in code handling types; mostly added
checks at the point of conversion from prog_data's data structures to
hlds_data's. If this is correct, why did you make the change? And if this
is not correct, why isn't it?

Since the rest of the diff is full of stuff dependent on this design
decision, I will finish the review once you and I agree on the types
here; there is not much point until then.

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