[m-dev.] for review: Aditi updates[6]

[Simon Taylor]

> 
> On 05-Jun-1999, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> > Index: doc/reference_manual.texi
> > + at node Aditi update syntax
> > + at subsubsection Aditi update syntax
> > +
> > +The Melbourne Mercury compiler provides special syntax to specify updates
> > +of Aditi base relations.  For a predicate @samp{p/3}, the following update
> > +procedures are defined. Variables named @samp{DB at var{N}} are
> > + at samp{aditi__state} variables. When occuring as a pair at the end
> > +of an argument list, they have mode @samp{aditi_di, aditi_uo}.
> 
> This paragraph is a long way away from the actual examples.

I've moved everything except the first sentence of this
paragraph to after the declarations used in the examples.

> 
> Also, the things that follow are builtin goals, not procedures.
> (If they were procedures, then you could take their address, etc.,
> but you can't.)
> 
> If this subsection is describing special syntax, then it doesn't really
> belong in the "Implementation-dependent pragmas" section anymore.  So we
> may need to rethink the organization of the language reference manual.

Yes, I was thinking about that. I don't think the C interface, 
tabling and termination analysis belong under "Pragmas" either.
The interesting thing is that the language has these features,
not that there are pragmas to control them. I'll do that as a
separate change.

> > +It is currently up to the application to ensure that any modifications
> > +do not change the determinism of a base relation. Updates of relations
> > +with unique B-tree indexes are checked to ensure that a key is not given
> > +multiple values. The transaction will abort if this occurs.
> 
> s/indexes/indices/ ?

OK.

> > +Note: The implementation of Aditi updates is not yet complete.
> 
> What part is not yet complete?
> 
> Probably they should not be documented in the reference manual unless/until
> you can at least state what will work and what won't work.

Code generation for the closures and runtime support.
It can't hurt to include the documentation because it is
commented out anyway.

> > +The examples make use of the following declarations.
> > + at example
> > +:- pred p(aditi__state::aditi_ui, int::out, int::out) is nondet.
> > +:- pragma base_relation(p/3).
> > +
> > +:- func q(aditi__state::aditi_ui, int::out) = (int::out) is nondet.
> > +:- pragma base_relation(q/2).
> 
> I suggest naming the function `f' rather than `q'.

OK.

> > + at subsubheading Deletion
> ...
> > +When updating a predicate with type declaration
> > + at w{@samp{:- pred p(aditi__state, @var{Type1}, @dots{})}},
> > + at samp{@var{Closure}} must have type
> > + at w{@samp{aditi_top_down pred(aditi__state, @var{Type1}, @dots{})}},
> > +and inst @w{@samp{pred(unused, in, @dots{}) is semidet}}.
> > +
> > +When updating a function with type declaration
> > + at w{@samp{:- func p(aditi__state, @var{Type1}, @dots{}) = @var{Type2}}},
> > + at samp{@var{Closure}} must have type
> > + at w{@samp{aditi_top_down func(aditi__state, @var{Type1}, @dots{}) = @var{Type2}}},
> > +and inst @w{@samp{func(unused, in, @dots{}) = in is semidet}}.
> 
> s/When updating/When deleting from/ ?

OK.

> > +The @samp{aditi__state} argument of @samp{@var{Closure}} must have
> > +mode @samp{unused} -- it is not possible to call an Aditi
> > +relation from the deletion condition. All other arguments of
> > + at samp{@var{Closure}} must have mode @samp{in}.
> 
> s/--/---/

OK.

> > +        DeleteP = (aditi_top_down
> > +               pred(_::unused, X::in, Y::in) is semidet :-
> > +                        X = 2
> > +               ),
> > +        aditi_delete(pred p/3, DeleteP, DB0, DB).
> > +
> > +        DeleteQ = (aditi_top_down
> > +               func(_::unused, X::in) = (Y::in) is semidet :-
> > +                        X = 2
> > +               ),
> > +        aditi_delete(func q/2, DeleteQ, DB0, DB).
> > + at end example
> > +
> > +The type of @samp{Pred} is
> > + at w{@samp{aditi_top_down pred(aditi__state, int, int)}}.
> > +Its inst is @w{@samp{pred(unused, in, in) is nondet}}, as for a normal
> > +lambda expression.
> 
> s/Pred/DeleteP/ ?

Yep.

> > + at subsubheading Bulk insertion
> 
> There are sufficiently many @subsubheadings that I think there should be
> separate @nodes for each @subsubheading here, with an @menu to select
> between them.

See the comment above: "@c XXX texinfo doesn't have subsubsubsection."
I'll fix this when I reorganize the "Pragmas" chapter.

> > + at example
> > +aditi_bulk_insert(@var{PredOrFunc} @var{Name}/@var{Arity}, @var{Closure}, @var{DB0}, @var{DB}).
> > + at end example
> > +
> > +Insert all solutions of @samp{@var{Closure}} into the named relation.
> > + at samp{@var{PredOrFunc}} must be either @samp{pred} or @samp{func}.
> > +There must be a @w{@samp{:- pragma base_relation}} declaration for
> > + at w{@samp{@var{Name}/@var{Arity}}}.
> 
> Can `Name' be module-qualified?
> (Same question applies in a few places around here.)

Yes.

> > +The closure to produce the tuples to insert must have the same
> 
> I suggest s/to produce/for producing/

OK.


> 	example_1(DB0, DB) :-
> 		...
> 
> 	example_2(DB0, DB) :-
> 		...

Done.

> > +The type of @samp{Pred} is
> > + at w{@samp{aditi_bottom_up pred(aditi__state, int, int)}}.
> 
> Again, s/Pred/InsertP/ ?

Yep.

> > + at subsubheading Modification
> > +
> > + at example
> > +aditi_modify(
> > +        (@var{PredName}(@var{Var1}, @var{Var2}, @dots{}) = @var{Predname}(@var{Var3}, @var{Var4}, @dots{}) :-
> > +                @var{Goal},
> > +        ),
> > +        @var{DB0}, @var{DB}).

> As I mentioned in previous mail, I think using `=' here is not very nice
> syntax.

Changed to `==>' as you suggested.

> > +aditi_modify(@var{PredOrFunc} @var{PredName}/@var{Arity}, @var{Closure}, @var{DB0}, @var{DB}).
> > +
> > + at end example
> > +
> > +Modify tuples for which @samp{@var{Goal}} or @samp{@var{Closure}}.
> 
> That sentence is incomplete.

Modify tuples for which @samp{@var{Goal}} or @samp{@var{Closure}} succeeds.

> > +The @samp{aditi__state} arguments of @samp{@var{Closure}} must have
> > +mode @samp{unused} -- it is not possible to call an Aditi
> > +relation from the modification goal.
> 
> s/--/---/

OK.

> > Index: tests/invalid/aditi.m
> > ===================================================================
> 
> This file was not mentioned in the log message.
> 
> Also it would be helpful if the file had some comments
> explaining its purpose.

I've added it to the log message and added the comment 

	% Cut down version of extras/aditi/aditi.m containing 
	% declarations used by other tests.

> > Index: tests/invalid/aditi_update_errors.err_exp
> > ===================================================================
> > RCS file: aditi_update_errors.err_exp
> > diff -N aditi_update_errors.err_exp
> > --- /dev/null	Sat Jun  5 14:25:53 1999
> > +++ aditi_update_errors.err_exp	Fri Jun  4 11:54:16 1999
> > @@ -0,0 +1,210 @@
> > +aditi_update_errors.m:112: Error: wrong number of arguments (5; should be 3 or 4)
> > +aditi_update_errors.m:112:   in call to predicate `aditi_modify'.
> 
> "predicate" is not really the right word here.

Fixed.

> > --- ho_type_mode_bug.err_exp	1997/06/16 08:55:31	1.1
> > +++ ho_type_mode_bug.err_exp	1999/05/21 09:43:11
> > @@ -1,11 +1,9 @@
> >  ho_type_mode_bug.m:025: In clause for `my_foldl2((pred(in, in, out) is det), in, in, out, in, out)':
> > -ho_type_mode_bug.m:025:   in argument 1 of higher-order predicate call
> > -ho_type_mode_bug.m:025:   (i.e. in the predicate term):
> > +ho_type_mode_bug.m:025:   in argument 1 (i.e. the predicate term) of call to predicate `call/6':
> 
> Hmm, this seems like a small step backwards in terms of readability.
> How hard would it be to keep the old message?

The error message now reads:

ho_type_mode_bug.m:025: In clause for `my_foldl2((pred(in, in, out) is det), in,
 in, out, in, out)':
ho_type_mode_bug.m:025:   in argument 1 (i.e. the predicate term) of higher-orde
r predicate call:
ho_type_mode_bug.m:025:   mode error: variable `P' has instantiatedness `(pred(i
n, in, out) is det)',
ho_type_mode_bug.m:025:   expecting higher-order pred inst (of arity 5).

Keeping the original line wrapping is difficult because
hlds_out__write_call_arg_id does not have a context.
Adding a context there is probably the wrong fix - we should
eventually add better support for line wrapping in error messages
throughout the compiler without having to bodge it in everywhere.

Does the "(i.e. the predicate term)" really add anything here?

> > Index: tests/valid/aditi.m
> 
> A brief comment explaining the purpose of this module would be helpful.

Same as for tests/invalid/aditi.m.

> OK, that completes this review (hurray!).

Thanks Fergus.

> I'd like to see a relative diff for whatever changes you make to address
> the points raised in my review. 

Coming soon.

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