[m-rev.] for review: automatically generate all type_ctor_infos on non C backends

[Fergus Henderson]

On 20-Nov-2003, Peter Ross <pro at missioncriticalit.com> wrote:
> Index: compiler/make_hlds.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/make_hlds.m,v
> retrieving revision 1.452
> diff -u -r1.452 make_hlds.m
> --- compiler/make_hlds.m	20 Nov 2003 11:35:40 -0000	1.452
> +++ compiler/make_hlds.m	20 Nov 2003 21:21:42 -0000
> @@ -161,11 +161,30 @@
>  		InvalidTypes1 = no,
>  		module_info_types(Module2, Types),
>  		map__foldl3(process_type_defn, Types,
> -			no, InvalidTypes2, Module2, Module3, !IO)
> +			no, InvalidTypes2, Module2, Module3a, !IO)

The time when you start introducing names like "Module3a" is the time to
switch to state variable notation.  So IMHO this is OK for the purpose
of this patch if and only if it is followed by a patch to change this
procedure to use state variable notation.

Note that the way to introduce a state variable notation
for a local variable sequence is to use an explicit quanitifer:

	some [!Module] (
		... code using !.Module, !:Module, and !Module ...
	]

> +	% Add the special preds for the builtins

s/builtins/builtin types/

Actually, better clarify what you mean by "builtin".
(See further comments below.)

> Index: compiler/type_ctor_info.m
> @@ -106,45 +114,62 @@
...
> +	    SymName = qualified(TypeModuleName, TypeName),
> +	    ( 
> +		TypeModuleName = ModuleName,
> +		( list__member(TypeCtor, builtin_type_ctors) ->
> +		    compiler_generated_rtti_for_the_builtins(ModuleInfo),
> +		    TypeModuleName = unqualified(ModuleNameString),
> +		    TypeDefn = builtin_type_defn,
> +		    builtin_type_ctor(ModuleNameString, TypeName, TypeArity, _)

Please put the unification TypeDefn = builtin_type_defn after the call
to builtin_type_defn.  It's clearer (not to mention potentially more
efficient) if all of the tests precede any goals which can't fail.

It would help to have a comment at the start of this whole if-then-else,
explaining what the condition is trying to test, e.g. add

	%
	% Check if we should generate a type_ctor_info for this type
	%

after SymName = qualified(...).

> +		;
> +		    map__lookup(TypeTable, TypeCtor, TypeDefn),
> +		    hlds_data__get_type_defn_body(TypeDefn, TypeBody),
> +		    (
> +			TypeBody \= abstract_type(_)
> +		    ->
> +			\+ type_ctor_has_hand_defined_rtti(TypeCtor, TypeBody),
> +			( are_equivalence_types_expanded(ModuleInfo)
> +				=> TypeBody \= eqv_type(_) )
> +		    ;
> +			% type_ctor_infos need be generated for the builtin
> +			% types (which are declared as abstract types)
> +			compiler_generated_rtti_for_the_builtins(ModuleInfo),
> +			TypeModuleName = unqualified(ModuleNameString),
> +			( builtin_type_ctor(ModuleNameString,
>  					TypeName, TypeArity, _)
> +			; impl_type_ctor(ModuleNameString,
>  					TypeName, TypeArity, _)
>  			)
> +		    )
>  		)

This code is very unclear now.  Didn't it already check for
builtin_type_ctors above?  What is the difference between
calling builtin_type_ctor/4, and checking membership of
the list returned by builtin_type_ctors?

When you're talking about things which are "builtin", it is
very important to be clear about exactly what you mean by that.
This code uses different senses for "builtin_type_ctor" and
"builtin_type_ctors", which is very confusing.
                  ^

> +:- func builtin_type_defn = hlds_type_defn.
> +
> +builtin_type_defn = TypeDefn :-
> +	varset__init(TVarSet),
> +	Params = [],
> +	Body = abstract_type(non_solver_type),
> +	ImportStatus = local,
> +	NeedQualifier = may_be_unqualified,
> +	term__context_init(Context),
> +	hlds_data__set_type_defn(TVarSet, Params, Body,
> +			ImportStatus, NeedQualifier, Context, TypeDefn).

Please add some comments here, explaining what sense of "builtin"
this is referring to, what kind of type definition it will return,
and why this is suitable.

> Index: compiler/type_util.m
...
> +	% The list of type_ctors which are builtins.
> +:- func builtin_type_ctors = list(type_ctor).

See my comments above.

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