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

[Zoltan Somogyi]

On Mon, 6 Jan 2025 15:22:26 +1100, Julien Fischer <jfischer at opturion.com> wrote:
> > 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 missed that.

> I'm not opposed to doing so, although I will need to take a
> look at how extensively it is currently being used beforehand.

In that case, what we should probably do is introduce a new
predicate with the in, out order, and mark the old predicate obsolete
in its favor. The new one should have a more descriptive name than
"nondet_member". Would "return_members" or "return_members_one_by_one"
do?

> (nondet_range_member should use a similar ordering for the sake of
> consistency.)

Agreed.

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

Once we settled on what we should do for ranges.m, yes.

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

Is that documented anywhere? If not, it should be.

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

I knew the former, and expected the latter. It was a post-commit review
after all.

Zoltan.