[m-rev.] for review: type class constraint info in du functor descriptors

Fergus Henderson fjh at cs.mu.OZ.AU
Thu Feb 5 13:41:37 AEDT 2004
Previous message: [m-rev.] diff: fix typos in reference manual
Next message: [m-rev.] for review: type class constraint info in du functor descriptors
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
On 27-Jan-2004, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: compiler/mlds_to_gcc.m
...
> +:- pred build_tc_constr_type(gcc__type::out, io__state::di, io__state::uo)
> +	is det.
> +
> +build_tc_constr_type(MR_TypeClassConstraint) -->
> +	build_tc_constr_struct_type(5, MR_TypeClassConstraint5Struct),
> +	gcc__build_pointer_type(MR_TypeClassConstraint5Struct,
> +		MR_TypeClassConstraint).

There should be at least a comment here explaining where the magic number 5
comes from.

After staring at this code here and the code in mercury_type_info.h
for a while, I figured out that this code must be corresponding to the C code

  MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_5, 5);

  typedef MR_TypeClassConstraint_5Struct          MR_TypeClassConstraintStruct;
  typedef const MR_TypeClassConstraintStruct      *MR_TypeClassConstraint;

in mercury_type_info.h.  Could you please add this C code as a comment here,
as is done for the other types defined in mlds_to_gcc.m, so that the
correspondence is clearer?

> Index: compiler/rtti.m
> @@ -1345,6 +1384,9 @@
> +rtti__pred_or_func_to_string(predicate, "MR_PREDICATE").
> +rtti__pred_or_func_to_string(function,  "MR_FUNCTION").
> +
>  rtti__sectag_locn_to_string(sectag_none,   "MR_SECTAG_NONE").
>  rtti__sectag_locn_to_string(sectag_local,  "MR_SECTAG_LOCAL").
>  rtti__sectag_locn_to_string(sectag_remote, "MR_SECTAG_REMOTE").

This change (together with the changes to rtti_to_mlds.m to generate
these) requires appropriate support in the Java back-end, in particular
defining a PredFunc class in java/runtime/PredFunc.java analagous to
the way Sectag_Locn is defined in java/runtime/Sectag_Locn.java.

> +++ compiler/rtti_out.m	22 Jan 2004 23:33:16 -0000
> @@ -271,7 +271,7 @@
>  
>  output_type_class_instance_defn(Instance, !DeclSet, !IO) :-
>  	Instance = tc_instance(TCName, TCTypes, NumTypeVars, Constraints,
> -		MethodProcLabels),
> +		_MethodProcLabels),
>  	list__foldl2(output_maybe_pseudo_type_info_defn, TCTypes,
>  		!DeclSet, !IO),
>  	TCTypeRttiDatas = list__map(maybe_pseudo_type_info_to_rtti_data,
> @@ -301,21 +301,21 @@
>  			ConstraintIds, !IO),
>  		io__write_string("};\n", !IO)
>  	),
> -	TCInstanceMethodsRttiId = tc_rtti_id(
> -		type_class_instance_methods(TCName, TCTypes)),
> -	(
> -		MethodProcLabels = []
> -	;
> -		MethodProcLabels = [_ | _],
> -		MethodCodeAddrs = list__map(make_code_addr, MethodProcLabels),
> -		list__foldl2(output_code_addr_decls, MethodCodeAddrs,
> -			!DeclSet, !IO),
> -		output_generic_rtti_data_defn_start(TCInstanceMethodsRttiId,
> -			!DeclSet, !IO),
> -		io__write_string(" = {\n", !IO),
> -		list__foldl(output_code_addr_in_list, MethodCodeAddrs, !IO),
> -		io__write_string("};\n", !IO)
> -	),
> +%	TCInstanceMethodsRttiId = tc_rtti_id(
> +%		type_class_instance_methods(TCName, TCTypes)),
> +%	(
> +%		MethodProcLabels = []
> +%	;
> +%		MethodProcLabels = [_ | _],
> +%		MethodCodeAddrs = list__map(make_code_addr, MethodProcLabels),
> +%		list__foldl2(output_code_addr_decls, MethodCodeAddrs,
> +%			!DeclSet, !IO),
> +%		output_generic_rtti_data_defn_start(TCInstanceMethodsRttiId,
> +%			!DeclSet, !IO),
> +%		io__write_string(" = {\n", !IO),
> +%		list__foldl(output_code_addr_in_list, MethodCodeAddrs, !IO),
> +%		io__write_string("};\n", !IO)
> +%	),

There should be a comment here (not just in the CVS log message)
explaining why this code is commented out.

>  	TCDeclRttiId = tc_rtti_id(type_class_decl(TCName)),
>  	output_rtti_id_decls(TCDeclRttiId, "", "", 0, _, !DeclSet, !IO),
>  	TCInstanceRttiId = tc_rtti_id(type_class_instance(TCName, TCTypes)),
> @@ -336,15 +336,15 @@
>  		Constraints = [_ | _],
>  		output_rtti_id(TCInstanceConstraintsRttiId, !IO)
>  	),
> -	io__write_string(",\n\t", !IO),
> -	(
> -		MethodProcLabels = [],
> -		io__write_string("NULL", !IO)
> -	;
> -		MethodProcLabels = [_ | _],
> -		io__write_string("&", !IO),
> -		output_rtti_id(TCInstanceMethodsRttiId, !IO)
> -	),
> +%	io__write_string(",\n\t", !IO),
> +%	(
> +%		MethodProcLabels = [],
> +%		io__write_string("NULL", !IO)
> +%	;
> +%		MethodProcLabels = [_ | _],
> +%		io__write_string("&", !IO),
> +%		output_rtti_id(TCInstanceMethodsRttiId, !IO)
> +%	),

Likewise.

> Index: compiler/rtti_to_mlds.m
...
> +gen_type_class_decl_defn(TCDecl, RttiId, ModuleInfo, Init, SubDefns) :-
...
> +	(
> +		Supers = [],
> +		SuperDefns = [],
> +		SupersInit = gen_init_null_pointer(
> +			mlds__rtti_type(item_type(MethodIdsRttiId)))
> +	;
> +		Supers = [_ | _],
> +		list__map_foldl2(gen_tc_constraint(ModuleInfo,
> +			make_decl_super_id(TCName)), Supers, SuperRttiIds,
> +			counter__init(1), _, [], SuperConstrDefns),
> +		SuperArrayRttiId = tc_rtti_id(SuperArrayRttiName),
> +		ElementType = mlds__rtti_type(element_type(SuperArrayRttiId)),
> +		SuperArrayInit = gen_init_array(
> +			gen_init_cast_rtti_id(ElementType, ModuleName),
> +			SuperRttiIds),
> +		SuperArrayRttiName = type_class_decl_supers(TCName),
> +		rtti_id_and_init_to_defn(SuperArrayRttiId, SuperArrayInit,
> +			SuperDefn),
> +		list__append(SuperConstrDefns, [SuperDefn], SuperDefns),
> +		SupersInit = gen_init_null_pointer(
> +			mlds__rtti_type(item_type(MethodIdsRttiId)))
> +	),

The definition of "SupersInit" appears to be the same in both cases,
so it would be simpler to move that to after this switch on "Supers".

But I don't understand -- why is SupersInit always a null pointer?

> +gen_tc_constraint(ModuleInfo, MakeRttiId, Constraint, RttiId, !Counter,
> +		!Defns) :-
> +	Constraint = tc_constraint(TCName, Types),
> +	list__length(Types, Arity),
> +	counter__allocate(TCNum, !Counter),
> +	MakeRttiId(TCNum, Arity, RttiId),
> +	TCDeclRttiName = type_class_decl(TCName),
> +	module_info_name(ModuleInfo, ModuleName),
> +	TypeRttiDatas = list__map(maybe_pseudo_type_info_to_rtti_data, Types),
> +	gen_pseudo_type_info_array(ModuleInfo, TypeRttiDatas, PTIInits,
> +		PTIDefns),
> + 	Init = init_struct(mlds__rtti_type(item_type(RttiId)), [
> +		gen_init_tc_rtti_name(ModuleName, TCDeclRttiName),
> +		PTIInits
> + 	]),
> +	rtti_id_and_init_to_defn(RttiId, Init, ConstrDefn),
> +	list__append(PTIDefns, [ConstrDefn], NewDefns),
> +	list__append(!.Defns, NewDefns, !:Defns).

The last two lines would IMHO be better written as

	!:Defns = !.Defns ++ PTIDefns ++ [ConstrDefn].

Using infix ++ rather than a sequence of list__append operations makes
it easier to see the order in which the different parts will get placed,
avoiding bugs like the one in polymorphism.m that James Goddard recently
fixed.

> Index: runtime/mercury_type_info.h
...
> @@ -702,6 +686,44 @@
>  /*---------------------------------------------------------------------------*/
>  
>  /*
> +** A typeclass constraint asserts the membership of a possibly nonground
> +** vector of types in a type class, as one may find constraining a typeclass
> +** declaration, an instance declaration, or a predicate/function declaration.
> +**
> +** Type class constraints for type classes with arity N will be of type
> +** MR_TypeClassConstraint_N. Generic code will manipulate them as if they were
> +** of type MR_TypeClassConstraint, getting the actual number of arguments from
> +** MR_tc_constr_type_class_info->MR_tc_decl_id->MR_tc_id_arity.
> +**
> +** Note that the arity cannot be zero, so we do not have to worry about
> +** zero-size arrays. On the other hand, type classes with more than even two
> +** arguments can be expected to be very rare, so having five as a fixed limit
> +** should not be a problem. If it is, we can lift the limit by defining
> +** MR_TypeClassConstraint_N on demand for all N > 5.
> +**
> +** We will have to rethink this structure once we start supporting constructor
> +** classes.
> +*/
> +
> +#define MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(NAME, ARITY)                  \
> +    typedef struct MR_PASTE2(NAME, _Struct) {                               \
> +        MR_TypeClassDecl    MR_tc_constr_type_class;                        \
> +        MR_PseudoTypeInfo   MR_tc_constr_arg_ptis[ARITY];                   \
> +    } MR_PASTE2(NAME, Struct)
> +
> +MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_1, 1);
> +MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_2, 2);
> +MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_3, 3);
> +MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_4, 4);
> +MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint_5, 5);
> +
> +typedef MR_TypeClassConstraint_5Struct          MR_TypeClassConstraintStruct;

Shouldn't the definition of the generic version use MR_VARIABLE_SIZED?
i.e.

	MR_DEFINE_TYPECLASS_CONSTRAINT_STRUCT(MR_TypeClassConstraint, 
		MR_VARIABLE_SIZED);


Otherwise that all looks fine.

-- 
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
--------------------------------------------------------------------------
Previous message: [m-rev.] diff: fix typos in reference manual
Next message: [m-rev.] for review: type class constraint info in du functor descriptors
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
More information about the reviews mailing list