[m-rev.] for post-commit review: adjust documentation of the ranges module

[Zoltan Somogyi]

On 2024-03-30 02:22 +11:00 AEDT, "Julien Fischer" <jfischer at opturion.com> wrote:44
> --- a/library/ranges.m
> +++ b/library/ranges.m
> @@ -34,70 +34,71 @@
>      %
>  :- func empty = ranges.
> 
> -    % is_empty(Set):
> -    % Succeeds iff Set is the empty set.
> +    % is_empty(Set) is true iff Set is the empty set.
>      %
>  :- pred is_empty(ranges::in) is semidet.

This comment is not unique to this diff, but I wonder whether
using "iff" as a shorthand helps more people (by being brief)
than it hurts (by baffling anyone who does not know what it
stands for). Given that the positive effect is small while the
negative effect is large, I think it would be better to phase out
its use.

> -    % range(Min, Max) is the set of all integers from Min to Max inclusive.
> +    % range(Lo, Hi) is the set of all integers from Lo to Hi inclusive.
>      %
>  :- func range(int, int) = ranges.

Given how frequently half-open domains exist in many parts
of computer science, I would go explicit and say "both inclusive".
 
> -    % split(D, L, H, Rest) is true iff L..H is the first range
> -    % in D, and Rest is the domain D with this range removed.
> +    % split(Set, Lo, Hi, Rest) is true iff Lo..Hi is the first range
> +    % in Set, and Rest is the set Set with this range removed.
>      %
>  :- pred split(ranges::in, int::out, int::out, ranges::out) is semidet.

I would add something after "first" to remind readers that this means
the range containing the smallest integers in the set.

> -    % is_contiguous(R, L, H) <=> split(R, L, H, empty):
> -    % Test if the set is a contiguous set of integers and return the endpoints
> -    % of this set if this is the case.
> +    % is_contiguous(Set, Lo, Hi) is true iff Set is a contiguous set
> +    % of integers with endpoints Lo and Hi.
>      %
>  :- pred is_contiguous(ranges::in, int::out, int::out) is semidet.

I would say "is the set of all integers from Lo to Hi, both inclusive".

> -    % least(A, N) is true iff N is the least element of A.
> +    % least(Set, X) is true iff X is the least element of Set.
>      %
>  :- pred least(ranges::in, int::out) is semidet.
> 
> -    % greatest(A, N) is true iff N is the greatest element of A.
> +    % greatest(Set, X) is true iff X is the greatest element of Set.
>      %
>  :- pred greatest(ranges::in, int::out) is semidet.

For both of these, I would add "Fails if the set is empty".
It is obvious, but being clear is better than leaving a few people wondering.

> -    % Test set membership.
> +    % member(X, Set) is true iff X is a member of Set.
>      %
>  :- pred member(int::in, ranges::in) is semidet.

If this diff is here, that means that this module does not *order*
its exported predicates the same way as the other set predicates do.
You may want to fix that in your next diff.

> +    % search_range(X, Set, Lo, Hi):
> +    %
> +    % If X is in Set, then succeed, setting Lo and Hi to the endpoints
>      % of the range in which it is contained.
>      %
>  :- pred search_range(int::in, ranges::in, int::out, int::out) is semidet.

Again, I would add "Otherwise, fail".

> +    % union(SetA, SetB) contains all the integers in either SetA or SetB.
>      %
>  :- func union(ranges, ranges) = ranges.

I would say "union(SetA, SetB):  return the set that icontains all the integers in set SetA and SetB."

> -    % intersection(A, B) contains all the integers in both A and B.
> +    % intersection(SetA, SetB) contains all the integers in both SetA and SetB.
>      %
>  :- func intersection(ranges, ranges) = ranges.
> 
> -    % difference(A, B) contains all the integers which are in A but
> -    % not in B.
> +    % difference(SetA, SetB) contains all the integers which are in SetA but
> +    % not in SetB.
>      %
>  :- func difference(ranges, ranges) = ranges.
> 
> -    % restrict_min(Min, A) contains all integers in A which are greater
> +    % restrict_min(Min, Set) contains all integers in Set which are greater
>      % than or equal to Min.
>      %
>  :- func restrict_min(int, ranges) = ranges.

I would do the same rewording as above for all these functions,
and the next few that now follow the same scheme.

> -    % prune_to_next_non_member(A0, A, N0, N):
> +    % prune_to_next_non_member(Set0, Set, X0, X):
>      %
> -    % N is the smallest integer larger than or equal to N0 which is not
> -    % in A0. A is the set A0 restricted to values greater than N.
> +    % X is the smallest integer larger than or equal to X0 that is not
> +    % in Set0. Set is the set Set0 restricted to values greater than X.
>      %
>  :- pred prune_to_next_non_member(ranges::in, ranges::out,
>      int::in, int::out) is det.

"Set X to the smallest ..." ",,, and Set to the ..."

> -    % prune_to_prev_non_member(A0, A, N0, N):
> +    % prune_to_prev_non_member(Set0, Set, X0, X):
>      %
> -    % N is the largest integer smaller than or equal to N0 which is not
> -    % in A0. A is the set A0 restricted to values less than N.
> +    % X is the largest integer smaller than or equal to X0 which is not
> +    % in Set0. Set is the set Set0 restricted to values less than X.
>      %
>  :- pred prune_to_prev_non_member(ranges::in, ranges::out,
>      int::in, int::out) is det.

Likewise.

> -    % Shift a range by const C.
> +    % Shift a range by a constant C.
>      %
>  :- func shift(ranges, int) = ranges.
> -    % Dilate a range by const C.
> +    % Dilate a range by a constant C.
>      %
>  :- func dilation(ranges, int) = ranges.
> -    % Contract a range by const C.
> +    % Contract a range by a constant C.
>      %
>  :- func contraction(ranges, int) = ranges.
>

I had no idea what these functions do, and still don't.
And the function names don't help.

Zoltan.