[m-dev.] for review: MR_TypeInfo cleanup, part 3 (of 3)

[Fergus Henderson]

On 22-Mar-2000, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: runtime/mercury_type_info.c
>  int
> -MR_compare_type_info(Word t1, Word t2)
> +MR_compare_type_info(MR_TypeInfo t1, MR_TypeInfo t2)
>  {
> -	Word		*type_info_1;
> -	Word		*type_info_2;
> +	MR_TypeInfo	type_info_1;
> +	MR_TypeInfo	type_info_2;
>  	MR_TypeCtorInfo	type_ctor_info_1;
>  	MR_TypeCtorInfo	type_ctor_info_2;
> +	MR_TypeInfo	*arg_vector_1;
> +	MR_TypeInfo	*arg_vector_2;

Should arg_vector_1 and arg_vector_2 be declared
with type `MR_TypeParams' rather than `MR_TypeInfo *'?
Well, I suppose it doesn't matter much either way,
since `MR_TypeParams' is just a typedef for `MR_TypeInfo *'.
But MR_TypeParams does carry a small bit of additional information,
in the documentation of the MR_TypeParams typedef,
namely that the bounds start at 1 rather than at zero.

> +++ mercury_unify_compare_body.h	Wed Mar 22 12:59:20 2000
> +        case MR_TYPECTOR_REP_NOTAG_GROUND:
> +            type_info = (MR_TypeInfo) type_ctor_info->type_layout.
> +                layout_notag->MR_notag_functor_arg_type;

Would it be clearer to use the MR_pseudo_type_info_is_ground()
macro here, rather than just casting to MR_TypeInfo?

> +        case MR_TYPECTOR_REP_C_POINTER:
> +#ifdef	select_compare_code
> +            if ((void *) x == (void *) y) {
> +                return_answer(MR_COMPARE_EQUAL);
> +            } else if ((void *) x < (void *) y) {

That line is not portable.  Comparing `void *' pointers with `<' is not
guaranteed to compare the whole pointer.  On architectures with segmented
pointers, it may only compare the offset, not the segment.

It would be better to cast the pointers to `Unsigned', rather than `void *',
This is likely to be a bit more portable.  It also matches what
the comparison predicate for c_pointer in library/builtin.m does.

> diff -u -b -r1.12 mercury_trace_vars.c
> --- trace/mercury_trace_vars.c	2000/02/22 10:46:55	1.12
> +++ trace/mercury_trace_vars.c	2000/03/19 07:56:41
> -static	Word *
> +static	MR_TypeCtorInfo
>  MR_trace_ignored_type_ctors[] =
...
> +	ignore_type_ctor_count =
> +		sizeof(MR_trace_ignored_type_ctors) / sizeof(Word *);
> -	ignore_type_ctor_count = sizeof(MR_trace_ignored_type_ctors)
> -					/ sizeof(Word *);

Here you've changed the element type from `Word *' to
`MR_TypeCtorInfo' in the declaration for MR_trace_ignored_type_ctors,
but not in the code where you compute that array's size.

It would be more maintainable to use

	ignore_type_ctor_count = sizeof(MR_trace_ignored_type_ctors)
				/ sizeof(MR_trace_ignored_type_ctors[0]);

Alternatively, you could define an MR_ARRAY_LENGTH() macro in
runtime/mercury_std.h,

	#define MR_ARRAY_LENGTH(array) (sizeof(array) / sizeof(array[0]))

and then use it like so:

	ignore_type_ctor_count = MR_ARRAY_LENGTH(MR_trace_ignored_type_ctors);

-- 
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.
--------------------------------------------------------------------------
mercury-developers mailing list
Post messages to:       mercury-developers at cs.mu.oz.au
Administrative Queries: owner-mercury-developers at cs.mu.oz.au
Subscriptions:          mercury-developers-request at cs.mu.oz.au
--------------------------------------------------------------------------