[m-dev.] cvs diff: type_info code.

[Fergus Henderson]

Tyson Richard DOWD, you wrote:
> 
> Fergus, could you please review this?

Sure.

BTW, your code was generally very well-documented --- thanks.

General comment: I'm not very comfortable with all this unnamed type
stuff.  If you can get at it using type_of/1, and do all this sort of
stuff with it, then I think perhaps it would be better to name it.  The
name to use would be `void'; it should probably be declared and defined
in mercury_builtin.m.

I think it would also be a good idea to enable the code in typecheck.m
that reports errors for uninstantiated type variables, with the error
changed to a warning (e.g. "Warning: type defaults to `void'.").

It would probably also be a good idea to avoid the special representation
as null typeinfo pointers and instead just use an ordinary typeinfo.

But don't worry about all that before committing.  I might have a go
at doing that stuff after you've committed it.

> +typedef struct mercury_construct_info {
> +	int vector_type;
> +	int arity;
> +	Word *functors_vector;
> +	Word *argument_vector;
> +	Word primary_tag;
> +	Word secondary_tag;
> +	ConstString functor_name;
> +} construct_info;

I think the coding standard says typedefs should be MixedCase.

> +static int	get_num_functors(Word type_info); 
> +static Word 	copy_argument_typeinfos(int arity, Word type_info,
> +				Word *arg_vector);
> +static bool 	get_functors_check_range(int functor_number, Word type_info, 
> +				construct_info *info);
> +static void	copy_arguments_from_list_to_vector(int arity, Word arg_list, 
> +				Word term_vector);
> +static int	typecheck_arguments(Word type_info, int arity, 
> +				Word arg_list, Word* arg_vector);
> +static int 	get_functor_info(Word type_info, int functor_number, 
> +				construct_info *info);
> +static Word 	collapse_equivalences(Word maybe_equiv_type_info);
> +").

These should not be static, because if things are compiled with
--split-c-files, or if the pragma C code fragments that call them
are cross-module-inlined, then it will break.

> +:- pragma c_code(type_of(Type::unused) = (TypeInfo::out),
> +	will_not_call_mercury, " 
> +{
> +	/* 
> +	** Type isn't used in this c_code, but the compiler
> +	** gives a warning if you don't mention it.
> +	*/ 

s/Type/`Type'/

Actually, now that I think about it for a moment, that variable name is
misleading: you should also s/Type/Value/

> +		/* 
> +		** Check range of FunctorNum, get info for this
> +		** functor.
> +		*/
> +	save_transient_registers();
> +	SUCCESS_INDICATOR = get_functors_check_range(FunctorNumber,
> +			TypeInfo, &info);
> +	restore_transient_registers();
> +
> +		/* 
> +		** Type check list of arguments 
> +		*/
> +
> +	if (SUCCESS_INDICATOR) {
> +		save_transient_registers();
> +		SUCCESS_INDICATOR = typecheck_arguments(TypeInfo, info.arity, 
> +			ArgList, info.argument_vector);
> +		restore_transient_registers();
> +	}

It would be slightly more efficient to do

		/* 
		** Check range of FunctorNum, get info for this
		** functor, and then type check the list of arguments
		*/
	save_transient_registers();
	SUCCESS_INDICATOR =
		get_functors_check_range(FunctorNumber, TypeInfo, &info) &&
		typecheck_arguments(TypeInfo, info.arity, ArgList,
				info.argument_vector);
	restore_transient_registers();

> +	/*
> +	** typecheck_arguments:
> +	**
> +	** Given a list of univs (`arg_list'), and an vector of
> +	** type_infos (`arg_vector'), checks that they are all of the
> +	** same type. `arg_vector' may contain type variables, these
> +	** will be filled in by the type arguments of `type_info' -
> +	** a type_info

I think that comment is incomplete.

> +int
> +typecheck_arguments(Word type_info, int arity, Word arg_list, Word* arg_vector) 
> +{
> +	int i, success;
> +	Word arg_type_info, list_arg_type_info;
> +
> +	success = TRUE;
> +
> +		/* Type check list of arguments */
> +
> +	for (i = 0; success && i < arity; i++) {
> +		if (list_is_empty(arg_list)) {
> +			success = FALSE;
> +			break;
> +		}
> +		list_arg_type_info = field(0, (list_head(arg_list)), 
> +			UNIV_OFFSET_FOR_TYPEINFO);
> +
> +		arg_type_info = (Word) create_type_info(
> +			(Word *) type_info, (Word *) arg_vector[i]);
> +
> +		success = (mercury_compare_type_info(
> +			list_arg_type_info, arg_type_info) == COMPARE_EQUAL);
> +		arg_list = list_tail(arg_list);
> +	}
> +
> +		/* List should now be empty */
> +	if (!list_is_empty(arg_list)) {
> +		success = FALSE;
> +	}
> +
> +	return success;
> +}

The code would be simpler if you got rid of the `success' variable,
and just did

		if (list_is_empty(arg_list)) {
			return FALSE;
		}

in the loop and

	return list_is_empty(arg_list);

at the end.

> +void
> +copy_arguments_from_list_to_vector(int arity, Word arg_list, Word term_vector) 
> +{
> +	int i;
> +
> +	for (i = 0; i < arity; i++) {
> +		field(mktag(0), term_vector, i) = 
> +			field(0, list_head(arg_list), UNIV_OFFSET_FOR_DATA);
> +		arg_list = list_tail(arg_list);
> +	}
> +}

How come one field has `mktag(0)' and the other uses just `0'?
Please be consistent.

> +bool
> +get_functors_check_range(int functor_number, Word type_info, 
> +	construct_info *info)
> +{
> +	bool success;
> +
> +		/* Check range of functor_number */
> +	success = (functor_number <= get_num_functors(type_info) 
> +		&& functor_number > 0);
> +
> +		/* Get functors vector, check for specials */
> +	if (success) {
> +		success = get_functor_info(type_info, functor_number, info);
> +	}
> +	return success;
> +}


The code would be simpler if you just did

	return	functor_number <= get_num_functors(type_info) &&
		functor_number > 0 &&
		get_functor_info(type_info, functor_number, info);

> +Word 
> +copy_argument_typeinfos(int arity, Word type_info, Word *arg_vector)
> +{
> +	Word type_info_list, *functors;
> +
> +	type_info_list = list_empty(); 
> +
> +	restore_transient_registers();

The restore_transient_registers() call should come before the call
to list_empty().  Otherwise it could break if compiled with --no-tags.

> +int 
> +get_num_functors(Word type_info)
> +{
> +	Word *base_type_functors;
> +	int Functors;
> +
> +	base_type_functors = MR_BASE_TYPEINFO_GET_TYPEFUNCTORS(
> +		MR_TYPEINFO_GET_BASE_TYPEINFO((Word *) type_info));
> +
> +	switch ((int) MR_TYPEFUNCTORS_INDICATOR(base_type_functors)) {
...
> +		case MR_TYPEFUNCTORS_UNIV:
> +			fatal_error(""std_util:get_num_functors :""
> +				"" found univ"");
> +		break;
> +
> +		default:
> +			fatal_error(""std_util:get_num_functors :""
> +				"" unknown indicator"");

Hmm, shouldn't you just return Functors = -1 for those cases?

Also, the indentation of your `break' statements is non-standard here
and elsewhere.

> +	 * If the substituted type parameters from the term_type_info
> +	 * were type variables, they will be replaced with references
> +	 * to the unnamed type (''/0).

I don't understand why that is done and I don't think it should be done.

> +typedef struct {
> +	Word enum_or_comp_const;
> +	Word num_sharers;		
> +	ConstString functor1;
> +/* other functors follow, num_sharers of them.
> +** 	ConstString functor2;
> +** 	...
> +*/
> +} MR_TypeLayout_EnumVector;
> +
> +#define MR_TYPELAYOUT_ENUM_VECTOR_NUM_FUNCTORS(Vector)			\
> +	((MR_TypeLayout_EnumVector *) (Vector))->num_sharers
> +
> +#define MR_TYPELAYOUT_ENUM_VECTOR_FUNCTOR_NAME(Vector, N)		\
> +	((ConstString) (((Word *) 					\
> +		&(((MR_TypeLayout_EnumVector *) (Vector))->functor1))	\
> +		[(N) - 1]))

The parenthesization and casts are quite confusing here.

Why not just

    #define MR_TYPELAYOUT_ENUM_VECTOR_FUNCTOR_NAME(Vector, N)		\
	( (&((MR_TypeLayout_EnumVector *)(Vector))->functor1) [(N) - 1] )

?

Can you please address the above issues, and then send around another diff?

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