[m-dev.] for review: cleanup of tabling

[Fergus Henderson]

On 30-Dec-1999, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:

private_builtin.m:
> +		subgoal = MR_table_allocate(MR_Subgoal);

That is not a function-like macro (since the argument
is a type name, not an expression), and so the macro
name should be in upper case.  For consistency with
the names used elsewhere (e.g. MR_NEW, MR_GC_NEW),
it should be called MR_TABLE_NEW.

> +		table->MR_subgoal = subgoal;
> +
>  		subgoal->status = MR_SUBGOAL_INACTIVE;
>  		subgoal->leader = NULL;
...
>  		subgoal->generator_maxfr = MR_prevfr_slot(MR_maxfr);
>  		subgoal->generator_sp = MR_sp;
> -		MR_SUBGOAL(T0) = subgoal;

Any particular reason to move that assignment?

I think it makes more sense to wait until the subgoal
is fully constructed before assigning it to table->MR_subgoal.

>  :- pragma c_code(table_nondet_answer_is_not_duplicate(T::in),
>  		will_not_call_mercury, "
...
> -	is_new_answer = (*((Word *) T) == MR_ANS_NOT_GENERATED);
> -	*((Word *) T) = MR_ANS_GENERATED;
> +
> +	is_new_answer = (table->MR_integer == 0);
> +	table->MR_integer = 1;	/* any nonzero value will do */

Hmm, why use 0 and 1 rather than MR_ANS_NOT_GENERATED
and MR_ANS_GENERATED? 

The comment here perhaps hints at the answer, but it's a bit cryptic;
what is it that any nonzero value will do?

I think some more documentation here would help.

> +	subgoal->num_ans += 1;

I'd suggest just

	subgoal->num_ans++;

> -	answer_node = table_allocate_bytes(sizeof(MR_AnswerListNode));
> -	answer_node->answer_num = table->num_ans;
> -	answer_node->answer_data = 0;
> +	answer_node = MR_table_allocate(MR_AnswerListNode);
> +	answer_node->answer_num = subgoal->num_ans;
> +	answer_node->answer_data.MR_integer = 0;
>  	answer_node->next_answer = NULL;

The setting of the MR_Integer field to zero is a little
mysterious; a comment would probably help.

>  :- pragma c_code(table_save_float_ans(T::in, Offset::in, F::in),
>  		will_not_call_mercury, "
> -	MR_TABLE_SAVE_ANSWER(Offset, T, float_to_word(F),
> +	MR_TrieNode	table;
> +
> +	table = (MR_TrieNode) T;
> +	/* XXX F vs float_to_word(F) */
> +	MR_TABLE_SAVE_ANSWER(table, Offset, F,
>  		mercury_data___type_ctor_info_float_0);
>  ").

The XXX here is right -- this code is buggy.
It should be `float_to_word(F)' rather than `F'.

`F' here has the C type `Float', since table_save_float_ans
is declared with the third argument of Mercury type `float',
and MR_TABLE_SIZE_ANSWER() will assign its third argument
to a variable of type `Word'; assigning a Float to a Word
will result in truncation.

>  :- pragma c_code(table_restore_float_ans(T::in, Offset::in, F::out),
>  		will_not_call_mercury, "
> -	F = word_to_float(MR_TABLE_GET_ANSWER(Offset, T));
> +	MR_TrieNode	table;
> +
> +	table = (MR_TrieNode) T;
> +	/* XXX F = word_to_float(MR_TABLE_GET_ANSWER(table, Offset)); */
> +	F = MR_TABLE_GET_ANSWER(table, Offset);
>  ").

Likewise here the XXX comment is right --
you need to use word_to_float().

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