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

[Julien Fischer]

On Sat, 30 Mar 2024, Zoltan Somogyi wrote:

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

I have no objection to that.

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

Done.

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

Done.

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

Done.

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

Done.

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

No, it doesn't order its exported predicates as per the other set
modules. I intend to fix that in subsequent change.

>
>> +    % 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".

Done.

>> +    % 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."

Done.

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

Done.

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

Done.

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

Done.

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

I know; I will deal with these after I have some more extensive testing
of this module in place; among other things contraction appears to
violate the invariants of the ranges types.

Julien.