[m-rev.] for review: reversing operator priorities

[Zoltan Somogyi]

On Tue, 8 Nov 2022 20:02:56 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
> > We probably should keep the old form of the ops module around,
> > possibly named old_ops.m or prolog_ops.m, for anyone who
> > wants it. I will make the necessary changes, including to NEWS,
> > once we get to a concensus on this.
> 
> I suggest adding old_ops to extras (similar to what we until recently
> had with old_term_parser.)

Done, both for ops.m, and mercury_term_parser.m.

> >      % An class describes what structure terms constructed with an operator
> >      % of that class are allowed to take.
> > +    %
> > +    % XXX OPS For binary ops, we should specify the arg priorities as one of
> > +    % left_assoc, right_assoc and non_assoc.
> > +    % XXX OPS For unary ops, we should specify the arg priority using
> > +    % the arg_priority type.
> 
> I'm not quite sure what you are proposing here.

I deleted those lines.


> > -        % The maximum priority of an operator appearing as the top-level
> > +        % Returns the loosest priority.
> > +        % XXX OPS Should this be loosest_OP_priority?
> 
> For consistency with the other method names?  Yes, although that does
> then leave lookup_operator_term being the odd one out.

My main motivation was that we need a priority that is one looser
than the loosest op priority, for contexts that can accept terms
with the loosest priority op as their top functor. This means that
the "loosest priority" cannot be the same as the "loosest op priority",
and the method name shouldn't imply that the loosest priority overall
can be assigned to an operator.

> > +        %
> > +    func loosest_priority(Table) = ops.priority,
> > +
> > +        % Returns the tightest priority.
> > +        % XXX OPS Should this be tightest_OP_priority?
> > +        %
> > +    func tightest_priority(Table) = ops.priority,
> > +
> > +        % XXX OPS Should there be a separate method for comma_priority?
> > +        % All users of this module need it, since they compute arg_priority
> > +        % from it.
> 
> Doesn't that assume that comma is in your operator table?

It is an operator in all of our use cases, and before this diff, all of them had
crude workarounds for the absence of that method from the typeclass.
I added the comma priority method, but documented how it should be implemented
for op_tables in which there is no comma method. This is inelegant, but
less inelegant than the current workarounds.

> > +% XXX OPS This predicate is unused.
> 
> Specifically, it is unused by the Mercury compiler; user code
> may have a use for it.

I updated the comment.

> > +decrement_priority(prio(P)) = prio(DecP) :-
> > +    trace [compile_time(flag("enforce_ops_priority_bounds"))] (
> > +        ( if prio(P) = mercury_op_table_loosest_priority then
> > +            unexpected($pred, "decrementing loosest priority")
> > +        else
> > +            true
> > +        )
> > +    ),
> > +    ( if P = 0u then
> > +        unexpected($pred, "decrementing 0")
> > +    else
> > +        DecP = P - 1u
> > +    ).
> 
> That fine otherwise; I only gave your conversion of the priorities in
> the table a cursory glance -- I'm assuming a bootcheck would catch
> any non-obvious errors.

I converted the numeric priorities using a shell script repeatly
invoking expr 1500 - num, so that table should be ok, and of course
bootchecks agree.

The attached updated diff and log message includes not just the
above fixes, but also action on many of the "XXX OPS" noted in the
original diff. There are also new XXXs, including one about why some
predicates in the standard library that should be able to handle arbitrary
op tables but are currently unable to do so, despite the documentation
implying that they can. It also includes the update to NEWS. I would
appreciate post-commit feedback on those parts of the diff.

Zoltan.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log.oprev2
Type: application/octet-stream
Size: 3372 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20221110/3d444111/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DIFF.oprev2
Type: application/octet-stream
Size: 184700 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20221110/3d444111/attachment-0003.obj>