[m-rev.] for post-commit review: begin addressing issues in the ranges module

[Julien Fischer]

On Mon, 6 Jan 2025 at 03:22, Zoltan Somogyi <zoltan.somogyi at runbox.com> wrote:

> My review is attached.
>
> > @@ -93,6 +96,19 @@
> >      %
> >  :- pred nondet_member(int::out, ranges::in) is nondet.
> >
> > +    % nondet_range_member(Lo, Hi, Set):
> > +    %
> > +    % Nondeterministically produce each range in Set.
> > +    % Each time this call succeeds, Lo and Hi will be bound to
> > +    % the smallest and largest integers respectively in a range in Set.
> > +    %
> > +:- pred nondet_range_member(int::out, int::out, ranges::in) is nondet.
> > +
> > +    % Obsolete synonym for nondet_range_member/3.
> > +    %
> > +:- pred range_member(int::out, int::out, ranges::in) is nondet.
> > +:- pragma obsolete(pred(range_member/3), [nondet_range_member/3]).
>
> Having outputs precede inputs in the argument list is not something that
> we normally do.

I know.

> Of course, list.member does it, but it came from Prolog.
> I would prefer it if the new predicates, nondet_member and
> nondet_range_member, had the ranges arg before the int arg or args.

nondet_member is *not* a new predicate. Altering its argument ordering will be
a breaking change. I'm not opposed to doing so, although I will need to take a
look at how extensively it is currently being used beforehand.
(nondet_range_member should use a similar ordering for the sake of
consistency.)

We should probably look at adding something like nondet_member to all the set
modules as a synonym for the '(out, in) is nondet' mode of member (provided
the argument re-ordering goes ahead.)

> Also, nondet_range_member is ambiguous. It could, and does, mean
> "returns each range in a set of ranges", but it could also be read as
> "return each *int* in a range". The signature rules that out, but
> it would be nice if we had a name that has no such ambiguity. Unfortunately,
> I can't think of one right now.

The usual convention in this module is that predicates with "range" in their
name operate on ranges (e.g. range_foldl) and those without it operate on
individual values in the set.

> > @@ -575,10 +638,31 @@ nondet_member(N, As) :-
> >  :- pragma foreign_export("Java",
> >      ranges.insert(in, in) = out, "insert").
> >
> > -insert(N, As) = union(As, range(N, N)).
> > -insert(N, As, Bs) :- Bs = insert(N, As).
> > +insert(N, As) = union(As, make_singleton_set(N)).
> > +insert(N, As, Bs) :-
> > +    Bs = insert(N, As).
>
> Please implement the function in terms of the predicate.

Done.

> > +insert_list(Es, Set0) = Set :-
> > +    insert_list(Es, Set0, Set).
> > +
> > +insert_list([], !Set).
> > +insert_list([E | Es], !Set) :-
> > +    !:Set = insert(E, !.Set),
> > +    insert_list(Es, !Set).
>
> Why not make this "insert(E, !Set)" now?

Done.

> > -delete(N, As) = difference(As, range(N, N)).
> > +%---------------------%
> > +
> > +delete(N, As) = difference(As, make_singleton_set(N)).
> > +delete(N, As, Bs) :-
> > +    Bs = delete(N, As).
> >
> > +delete_list(SetA, SetB) = Set:-
> > +    delete_list(SetA, SetB, Set).
> > +
> > +delete_list([], !Set).
> > +delete_list([E | Es], !Set) :-
> > +    !:Set = delete(E, !.Set),
> > +    delete_list(Es, !Set).
>
> Both of the above comments apply here as well, and in some later
> operations (union, intersect etc).

Most of the code in this module was originally written in a highly
functional style; unpicking that for union, intersect etc will be
a separate change.

> > -    % intersection(A, B) = A /\ B
> > +    % intersect(A, B) = A /\ B
>
> I know this is old code, but that comment is useless. It defines an
> operation that has a meaningful and well-understood name in terms of
> a symbol that has many possible meanings in different contexts, none
> of which are really applicable here. (The usual math notation for
> set intersection is an upside-down U, which is round, not pointy like /\).

I address this in a later change.

> > @@ -976,11 +1072,16 @@ to_sorted_list_2(L, H, Ints) =
> >
> >  %---------------------------------------------------------------------------%
> >
> > +:- pragma foreign_export("C",    ranges.count(in) = out, "ML_ranges_count").
> > +:- pragma foreign_export("Java", ranges.count(in) = out, "count").
> > +
> > +count(nil) = 0.
> > +count(range(L, U, Rest)) = (U - L) + size(Rest).
>
> This is not tail recursive. It does not matter much, but the code can be
> written in two minutes.

Done.

> > --- a/tests/hard_coded/test_ranges.exp
> > +++ b/tests/hard_coded/test_ranges.exp

...

> In the future, can you please separate commits that rename things
> in test cases (whose effects on .exp files don't need to be checked closely)
> from actual new test code (whose effects do need to be checked).

Shall do.

Julien.