[m-rev.] for review: comparison of preds/funcs

[Zoltan Somogyi]

On 22-Apr-2002, Mark Brown <dougl at cs.mu.OZ.AU> wrote:
> +#define	start_label		compare_rep_start
> +#define	call_user_code_label	call_compare_rep_in_proc
> +#define	type_stat_struct	MR_type_stat_mer_compare
> +#define	attempt_msg		"attempt to compare representation "
> +#define	select_compare_code
> +#define include_compare_rep_code
> +#define	entry_point_is_mercury

The indentation here is inconsistent.

You defined type_stat_struct to be the same as for the ordinary compare/3,
both here and in the C version, which means that the statistics for compare/3
and compare_representation/3 will be conflated.

> +#include "mercury_unify_compare_body.h"
> +
> +#undef  DECLARE_LOCALS
> +#undef  initialize
> +#undef  return_answer
> +#undef  start_label
> +#undef	call_user_code_label
> +#undef  type_stat_struct
> +#undef  attempt_msg
> +#undef	select_compare_code
> +#undef	include_compare_rep_code
> +#undef	entry_point_is_mercury

The indentation here is inconsistent.

> +#define	start_label		compare_rep_func_start
> +#define	call_user_code_label	call_compare_rep_in_func
> +#define	type_stat_struct	MR_type_stat_c_compare
> +#define	attempt_msg		"attempt to compare representation"
> +#define	select_compare_code
> +#define include_compare_rep_code

The indentation here is inconsistent.

> +#include "mercury_unify_compare_body.h"
> +
> +#undef  DECLARE_LOCALS
> +#undef  initialize
> +#undef  return_answer
> +#undef  start_label
> +#undef	call_user_code_label
> +#undef  type_stat_struct
> +#undef  attempt_msg
> +#undef	select_compare_code
> +#undef	include_compare_rep_code

The indentation here is inconsistent.

> +	x_layout = x->MR_closure_layout;
> +	y_layout = y->MR_closure_layout;
> +
> +	x_proc_id = &x_layout->MR_closure_id->MR_closure_proc_id;
> +	y_proc_id = &y_layout->MR_closure_id->MR_closure_proc_id;
> +	if (x_proc_id  < y_proc_id) {
> +		return MR_COMPARE_LESS;
> +	} else if (x_proc_id > y_proc_id) {
> +		return MR_COMPARE_GREATER;
> +	}

You should document that this works as long as the MR_closure_proc_ids
cannot be relocated, which is now guaranteed, but may not be true in the
future.

I am not sure whether the equality of the *contents* of MR_closure_proc_id
guarantees equality of their addresses.

> +test(SA - A, SB - B) -->
> +	io__write_string(SA),
> +	io__nl,
> +	io__write_string(SB),
> +	io__nl,
> +	{ std_util__compare_representation(Res, A, B) },
> +	(
> +		{ Res = (=) }
> +	->
> +		[]
> +	;
> +		io__write_string("not ")
> +	),
> +	io__write_string("equal\n\n").

Document you don't simply output Res, here and in the other test cases.

> +	(
> +		{ Res = (=) }
> +	->
> +		[]
> +	;
> +		io__write_string("maybe not ")
> +	),

Why the maybe?

Apart from that, the change is fine, so commit it once you have addressed my
comments.

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