[m-dev.] cvs diff: type_info code (round 2)

[Fergus Henderson]

Tyson Richard DOWD, you wrote:
> 
> 
> Here's the second round of this diff.
> 
> ===================================================================
> 
> +       % The `type_info' type - allows access to type information.
> +       %
> +       % A type_info represents the type of a variable.
> +       % 
> +       % It is possible for the type of a variable to be an unbound type
> +       % variable, this is represented as the type 'void'/0. 'void' is
> +       % considered a special (builtin) type - it is not a discriminated
> +       % union, so get_nth_functor/5 and the function construct/3 will
> +       % fail if used upon this type.
> +
> +:- type type_info == c_pointer.

That should be an abstract type.
The definition of the type should be in the implementation section.

> +	% TypeInfo = type_info(Data) returns the type_info (TypeInfo) of
> +	% the type of Data.
> +
> +:- func type_of(T) = type_info.
> +:- mode type_of(unused) = out is det.

I think the comment would be clearer if you wrote just

	% type_info(Data) returns the type_info for the type of Data.

> +	% num_functors(TypeInfo) returns the number of different
> +	%
> +	% Functors for the type specified by TypeInfo, or -1 if the type
> +	% is not a discriminated union type.

Delete the blank line, and s/Functors/functors/

> +:- pred get_nth_functor(type_info::in, int::in, string::out, int::out,
> +		list(type_info)::out) is semidet.
> +
> +	% construct(TypeInfo, N, Args) = Term
> +	%
> +	% Returns a term of the type specified by TypeInfo (or possibly
> +	% an instance thereof, if that type is polymorphic) whose
> +	% functor is the Nth functor of TypeInfo, and whose arguments
> +	% are given by Args.  Fails if the type is not a discriminated
> +	% union type, or if N is out of range, or if the number of
> +	% arguments doesn't match the arity of the Nth functor of the
> +	% type, or if the types of the arguments doesn't match the
> +	% expected argument types for that functor.

Delete the parenthetical remark "(or possibly an instance thereof, if
that type is polymorphic)".  TypeInfos always represent ground types.

> +:- pragma(c_header_code, "
> +
> +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;

s/Mercury_Construct_Info/Construct_Info_Struct/g
s/Construct_Info/ML_Construct_Info/g

> +static int 	get_functor_info(Word type_info, int functor_number, 
> +				Construct_Info *info);

That shouldn't be declared static, because of `--split-c-files',
cross-module inlining, etc.

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

Why do you collapse equivalences here -- is that just for efficiency,
or is it needed for correctness?  A comment here would be good.

> +	case MR_TYPEFUNCTORS_EQUIV:
> +		{
> +			Word *equiv_type;
> +			equiv_type = (Word *) MR_TYPEFUNCTORS_EQUIV_TYPE(
> +					base_type_functors);
> +			return get_functor_info((Word)
> +					create_type_info((Word *) type_info, 
> +							equiv_type),
> +					functor_number, info);
> +		}
> +		break;

The coding standard says that should be indented as

	case MR_TYPEFUNCTORS_EQUIV: {
		Word *equiv_type;
		equiv_type = (Word *) MR_TYPEFUNCTORS_EQUIV_TYPE(
				base_type_functors);
		return get_functor_info((Word)
				create_type_info((Word *) type_info, 
						equiv_type),
				functor_number, info);
		break;
	}

Since the last statement is a return statement,
I personally wouldn't put the `break' there.

> +int
> +ML_typecheck_arguments(Word type_info, int arity, Word arg_list,
> +		Word* arg_vector) 
> +{
> +	int i, success;
> +	Word arg_type_info, list_arg_type_info;
> +
> +		/* Type check list of arguments */
> +
> +	for (i = 0; success && i < arity; i++) {

That's a bug.  The success variable is uninitialized.
I suggest that you get rid of it (as I said originally).
You would need to change

> +		success = (mercury_compare_type_info(
> +			list_arg_type_info, arg_type_info) == COMPARE_EQUAL);

in the loop body to

		if (mercury_compare_type_info(list_arg_type_info,
			arg_type_info) != COMPARE_EQUAL)
		{
			return FALSE;
		}

> +	/*
> +	** ML_collapse_equivalences:
> +	**
> +	** Keep looking past equivalences until the there are no more.

Does this collapse equivalences in the argument typeinfos?
It would be worth mentioning that whether or not it does
in the documentation.

> +		case MR_TYPEFUNCTORS_EQUIV:
> +			{
> +				Word *equiv_type;
> +				equiv_type = (Word *) 
> +					MR_TYPEFUNCTORS_EQUIV_TYPE(
> +						base_type_functors);
> +				Functors = ML_get_num_functors((Word)
> +						create_type_info((Word *) 
> +							type_info, equiv_type));
> +			}
> +			break;

Non-standard layout.

> -typedef struct mercury_expand_info {
> +typedef struct Mercury_Expand_Info {
>  	ConstString functor;
>  	int arity;
>  	Word *argument_vector;
>  	Word *type_info_vector;
>  	bool need_functor;
>  	bool need_args;
> -} expand_info;
> +} Expand_Info;

If you're changing that to fit the coding standard, then you should also do

	s/Mercury_Expand_Info/Expand_Info_Struct/g
	s/Expand_Info/ML_Expand_Info/g

> +		/* 
> +		** If it's still a variable, make it a reference to 'void'.
> +		*/
> +	if ((Word) arg_pseudo_type_info < TYPELAYOUT_MAX_VARINT) {
> +		return (Word *) (Word) &mercury_data___base_type_info_void_0;
> +	}

As discussed, I think the variable case is not possible.
Please change that to

	if (arg_pseudo_type_info == NULL) {
		return (Word *) (Word) &mercury_data___base_type_info_void_0;
	}
	if (arg_pseudo_type_info < TYPELAYOUT_MAX_VARINT) {
		fatal_error("unbound type variable")
	}

>  	for (i = 0; i <= arity; i++) {
>  		if (arg_pseudo_type_info[i] < TYPELAYOUT_MAX_VARINT) {
>  			type_info[i] = term_type_info[arg_pseudo_type_info[i]];
> +
> +			/* 
> +			** It's a variable, make it a reference to`void'.
> +			*/
>  			if (type_info[i] < TYPELAYOUT_MAX_VARINT) {
> -				fatal_error(""Error! Can't instantiate type variable."");
> +				type_info[i] = (Word) 
> +					&mercury_data___base_type_info_void_0;
>  			}

Ditto.

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