Tabling round 2 [2/2]

[Fergus Henderson]

On 13-Mar-1998, Oliver Hutchison <ohutch at students.cs.mu.OZ.AU> wrote:
> +#define NON_TABLE(T)  (*((NondetTable **)T))

You should document that macro.

I'd define it as

	#define NON_TABLE(T)  (*(NondetTable **)T)

The extra parentheses in your original version
just make it harder to parse, IMHO.

> +:- pragma c_code(table_setup(T0::in, T::out), will_not_call_mercury, "
> +	/* Init the table if this is the first time me see it */
> +	if (NON_TABLE(T0) == NULL) {
> +		NON_TABLE(T0) = (NondetTable *) table_allocate(
> +			sizeof(NondetTable));
> +		NON_TABLE(T0)->status = have_no_ans;
> +		NON_TABLE(T0)->answer_table = 0;

Is that `0' supposed to represent a null pointer?
If so, `NULL' (or if necessary `(Word) NULL') would be
a better way of writing it.

> +		NON_TABLE(T0)->num_ans = 0;
> +		NON_TABLE(T0)->answer_list = list_empty();
> +		NON_TABLE(T0)->ans_list_tail =
> +			&NON_TABLE(T0)->answer_list;
> +		NON_TABLE(T0)->suspend_list = list_empty();
> +		NON_TABLE(T0)->suspend_list_tail =
> +			&NON_TABLE(T0)->suspend_list;
> +		NON_TABLE(T0)->non_stack_bottom = curprevfr;
> +		NON_TABLE(T0)->det_stack_bottom = MR_sp;
> +	}
> +	T = T0;
> +").
> +
> +
> +table_return_all_ans(T, A) :-
> +	semipure table_return_all_ans_list(T, AnsList),
> +	list__member(Node, AnsList),
> +	semipure table_return_all_ans_2(Node, A).
> +
> +:- semipure pred table_return_all_ans_list(table, list(table)).
> +:- mode table_return_all_ans_list(in, out) is det.
> +
> +:- pragma c_code(table_return_all_ans_list(T::in, A::out),
> +		 will_not_call_mercury, "
> +	A = NON_TABLE(T)->answer_list;
> +").
> +
> +:- semipure pred table_return_all_ans_2(table, table).
> +:- mode table_return_all_ans_2(in, out) is det.
> +
> +:- pragma c_code(table_return_all_ans_2(P::in, A::out), 
> +		will_not_call_mercury, "
> +	A = (Word) &((AnswerListNode*) P)->ans;
> +").

Why do you cast P to AnswerListNode* instead of casting to NondetTable*
(via the NON_TABLE macro)?

Perhaps you should change the types of the arguments in the
type declaration.

> +:- pragma c_code(table_suspend(T::in, A::out), will_not_call_mercury, "

Some more documentation about the code for this predicate
would be helpful.

Why is it declared as `nondet'?

> +	A = 0;
> +	fail();	

Why assign to A if you're only going to immediately fail anyway?

According to the nonexistant documentation for nondet pragma C ;-),
the `fail()' here should be `FAIL()'.

> +typedef struct {
> +	NondetTable *table;
> +	Word non_stack_block_size;
> +	Word *non_stack_block;
> +	Word det_stack_block_size;
> +	Word *det_stack_block;
> +	
> +	Code *succ_ip;
> +	Word *s_p;
> +	Word *cur_fr;
> +	Word *max_fr;
> +
> +	Word changed;
> +	Word num_ans, new_num_ans;
> +	Word suspend_list;
> +	SuspendListNode *suspend_node;
> +	Word ans_list;
> +	AnswerListNode *ansNode;
> +} ResumeStackNode;

You should document this structure.

> +Integer ML_resumption_sp = -1;
> +Word ML_resumption_stack_size = 256;	/* Half the initial size of 
> +						the stack */

In units of words? bytes? megabytes?

> +Define_extern_entry(mercury__table_resume_1_0);

You need much more documentation about the implementation
of this predicate.

Instead of using r1 directly, you should define
macros in runtime/mercury_type_info.h
similar to the `unify_input1', etc. macros,
so that it works without -DCOMPACT_ARGS.

> +Declare_label(mercury__table_resume_1_0_ChangeLoop);
> +Declare_label(mercury__table_resume_1_0_ChangeLoopDone);
> +Declare_label(mercury__table_resume_1_0_SolutionsListLoop);
> +Declare_label(mercury__table_resume_1_0_AnsListLoop);
> +Declare_label(mercury__table_resume_1_0_AnsListLoopDone);
> +Declare_label(mercury__table_resume_1_0_SkipAns);
> +Declare_label(mercury__table_resume_1_0_RedoPoint);
> +
> +MR_MAKE_STACK_LAYOUT_ENTRY(mercury__table_resume_1_0);
> +
> +BEGIN_MODULE(table_module)
> +	init_entry(mercury__table_resume_1_0);
> +	init_label(mercury__table_resume_1_0_ChangeLoop);
> +	init_label(mercury__table_resume_1_0_ChangeLoopDone);
> +	init_label(mercury__table_resume_1_0_SolutionsListLoop);
> +	init_label(mercury__table_resume_1_0_AnsListLoop);
> +	init_label(mercury__table_resume_1_0_AnsListLoopDone);
> +	init_label(mercury__table_resume_1_0_SkipAns);
> +	init_label(mercury__table_resume_1_0_RedoPoint);
> +BEGIN_CODE
> +
> +Define_entry(mercury__table_resume_1_0);
...
> +	bt_redoip(maxfr) = LABEL(mercury__table_resume_1_0_RedoPoint);

I don't understand what is going on here,
but it seems that you are clobbering the value of
bt_redoip(maxfr) without saving the previous value stored there.
Is that a bug?

> +Define_label(mercury__table_resume_1_0_AnsListLoop);
> +	r1 = (Word) &ML_RESUME_VAR->ansNode->ans;
> +
> +	succeed();

Here you are calling succeed() from a procedure with determinism
`failure', i.e. code model model_semi.  This looks very suspicious.

> +	SUCCESS_INDICATOR = 0;

s/0/FALSE/

> +:- pragma c_code(table_new_ans_slot(T::in, Slot::out), 
> +		will_not_call_mercury, "
...
> +	Slot = (Word)&n->ans;

s/(Word)/(Word) /
               ^

(Likewise in lots of other places.)

runtime/mercury_string.h:
>  #define HASH_STRING_FUNC_BODY				\
> -	   int hash;					\
> -	   do_hash_string(hash, s);			\
> -	   return hash;
> +	   int _hash;					\
> +	   do_hash_string(_hash, s);			\
> +	   return _hash;

s/_hash/hash_string_result/g

> --- mercury_type_info.h	1998/03/11 05:58:46	1.5
> +++ mercury_type_info.h	1998/03/12 01:49:05
> @@ -779,5 +779,10 @@
>  
>  #define MR_make_array(sz) ((MR_ArrayType *) make_many(Word, (sz) + 1))
>  
> +
> +Word * MR_create_type_info(Word *, Word *);
> +int MR_compare_type_info(Word, Word);
> +Word MR_collapse_equivalences(Word);

It would be useful to add a pointer to the documentation:

	/* These functions are documented in mercury_type_info.c */

Perhaps the documentation should be moved, but I think just
adding a pointer is good enough.

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