[m-dev.] for review: recognise associativity assertions

[Fergus Henderson]

On 20-Dec-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> 
> Is it alright to check this code in?

I'd like to see a diff for your updated changes first
(preferably both a relative diff and complete diff).

> On 09-Dec-1999, Peter Ross <petdr at cs.mu.OZ.AU> wrote:
> > How about:
> >     If a predicate has been declared associative via a promise
> >     declaration, the compiler is required to rearrange the order of that
> >     call to the predicates arguments if it makes the containing
> >     predicate tail recursive.

"is required to" seems wrong; there isn't any statement in the
Mercury language reference manual which requires the compiler to
do that.  Perhaps you should just say "will" instead.

Also "the order of that call to the predicates arguments" doesn't
sound right.  At very least it is missing an apostrophe somewhere.
Perhaps something like "the order of the arguments
in calls to that predicate, if by so doing it makes the
containing predicate tail recursive." would be better instead?

> >     If this changes the performance
> >     characteristics of the call to the predicate, use
> >     `--no-accumulator-introduction' to turn the optimization off, or
> >     `--inhibit-accumulator-warnings' to turn ignore the warnings.

"to turn ignore" should be "to turn off".

Also, I think you should say something like
"In such situations, the compiler will issue this warning."
before that sentence, and then change "If this" to
"if this reordering"

> > > It would be a good idea to have a test case in tests/warnings
> > > to test this warning.
> > > 
> > Will do when this code is checked in.

You might as well commit the test case at the same time
as the change to the compiler, IMHO.

> > > > +assertion__is_associativity_assertion(AssertId, Module, CallVars,
> > > > +		AssociativeVars) :-
> > > > +	assertion__goal(AssertId, Module, Goal - GoalInfo),
> > > > +	equivalent(Goal - GoalInfo, P, Q),
> > > > +
> > > > +	goal_info_get_nonlocals(GoalInfo, UniversiallyQuantifiedVars),
> > > > +
> > > > +	P = some(_, _, conj(PCalls) - _) - _PGoalInfo,
> > > > +	Q = some(_, _, conj(QCalls) - _) - _QGoalInfo,
> > > > +
> > > > +	AssociativeVars = promise_only_solution(associative(PCalls, QCalls,
> > > > +				UniversiallyQuantifiedVars, CallVars)).
> > > 
> > > I think the code here should have a comment explaining why the
> > > call to promise_only_solution is needed and why it is guaranteed
> > > to only have one solution (if, indeed, it is -- from a brief perusal
> > > of the code it wasn't immediately clear to me that that was the case).
> > > 
> > There should be only one or zero solutions.  The call to list__perm
> > ensures this, as the pairs are not symmetric.
> > 
> > list__perm(Pairs, [(A - AB) - (B - A), (B - C) - (C - BC),
> >                 (AB - ABC) - (BC - ABC)]),

I'd like to see a diff with some improved comments in the source code.

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