for review: minimal model tabling

[Fergus Henderson]

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

library/private_builtin.m:
> %-----------------------------------------------------------------------------%
> 
> :- interface.
> 
> % This section of the module contains the predicates that are
> % automatically inserted by the table_gen pass of the compiler
> % into predicates that use tabling, and the types they use.
> %
> % The predicates fall into three categories:
> %
> % (1)	Predicates that manage the status of simple subgoals.
> %	A subgoal is simple if its predicate is model_det or model_semi,
> %	which means that its evaluation method must be something
> %	other than minimal model.
> %
> % (2)	Predicates that manage the status of model_non subgoals,
> %	which usually means that its evaluation method is minimal model.
> % 
> % (3)	Utility predicates that are needed in the tabling of both
> %	simple and nondet subgoals.

I think you should also add comments indicating the start of each of these
groups, i.e.

%
% Predicates that manage the status of simple subgoals.
%
...
%
% Predicates that manage the status of model_non subgoals.
%
...
%
% Utility predicates that are needed in the tabling of both
% simple and nondet subgoals.
%

in the appropriate places.  The big comment describing the
three sections is too far removed from the sections themselves,
so it's difficult for the reader to figure out which code belongs
to which section.

> :- pragma c_code(table_simple_is_complete(T::in), will_not_call_mercury, "
> #ifdef	MR_TABLE_DEBUG
> 	if (MR_tabledebug) {
> 		printf(""checking if %p is succeeded or failed: %d\\n"",
> 			(Word *) T, *((Word *) T));
> 	}
> #endif
> 	if (*((Word *) T) == MR_SIMPLETABLE_FAILED
> 	|| *((Word *) T) >= MR_SIMPLETABLE_SUCCEEDED)
> 	{
> 		SUCCESS_INDICATOR = TRUE;
> 	} else {
> 		SUCCESS_INDICATOR = FALSE;
> 	}
> ").

You could make that just

 	SUCCESS_INDICATOR =
		(  *((Word *) T) == MR_SIMPLETABLE_FAILED
		|| *((Word *) T) >= MR_SIMPLETABLE_SUCCEEDED)

I suppose it doesn't really matter much either way.

> 	% If the answer represented by the given answer table
> 	% has not been generated before by this subgoal,
> 	% succeed and remember the answer as having been generated.
> 	% If the answer has been generated before, fail.
> :- semipure pred table_nondet_answer_is_not_duplicate(ml_answer_table_node).
> :- mode table_nondet_answer_is_not_duplicate(in) is semidet.

Shouldn't that be impure rather than semipure?

> 	% The definitions of these two predicates are in the runtime system.
> :- external(table_nondet_suspend/2).
> :- external(table_nondet_resume/1).

It might be helpful to make that a little more specific:

 	% The definitions of these two predicates are in the runtime system,
	% in runtime/mercury_tabling.c.


That completes my review of your minimal model tabling changes.

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