[m-rev.] for review: make split_list/4, drop/3 and take/3 consistent.

[Julien Fischer]

On Sat, 4 Jun 2016, Mark Brown wrote:

> On Sat, Jun 4, 2016 at 5:15 PM, Julien Fischer <jfischer at opturion.com> wrote:
>>
>> For review by anyone.
>>
>> -----------------------------------------------------------------------
>
>> diff --git a/library/list.m b/library/list.m
>> index 4a18bcc..9c33899 100644
>> --- a/library/list.m
>> +++ b/library/list.m
>> @@ -206,34 +206,35 @@
>>  :- pred same_length3(list(T1)::in, list(T2)::in, list(T3)::in)
>>      is semidet.
>>
>> -    % split_list(Len, List, Start, End):
>> +    % split_list(N, List, Start, End):
>>      %
>> -    % splits `List' into a prefix `Start' of length `Len', and a remainder
>> -    % `End'. See also: take, drop and split_upto.
>> +    % splits `List' into a prefix `Start' of length `N', and a remainder
>> +    % `End'. Fails if `N' is not in 0 .. length(List).
>> +    % See also: take, drop and split_upto.
>
> I feel there should be some quotes around that last occurrence of
> List.

Done.

> Or perhaps just leave out the quotes entirely, as more recent
> parts of the documentation tend to do?

There's a lack of consistency as to whether we quote things in the
library documentation (among many things).  In the absence of the coding
guidlines saying anything on the matter, I just went for local
consistency.

>>      %
>>  :- pred split_list(int::in, list(T)::in, list(T)::out, list(T)::out)
>>      is semidet.
>>
>> -    % det_split_list(Len, List, Start, End):
>> +    % det_split_list(N, List, Start, End):
>>      %
>>      % A deterministic version of split_list, which aborts instead
>> -    % of failing if Len > length(List).
>> +    % of failing if `N' is not in 0 .. length(List).
>>      %
>>  :- pred det_split_list(int::in, list(T)::in, list(T)::out, list(T)::out)
>>      is det.
>>
>> -    % split_upto(Len, List, Start, End):
>> +    % split_upto(N, List, Start, End):
>>      %
>> -    % splits `List' into a prefix `Start' of length `min(Len,
>> length(List))',
>> +    % splits `List' into a prefix `Start' of length `min(N, length(List))',
>
> This comment should also account for the N < 0 case. (I presume people
> are happy with {split,take}_upto keeping their existing semantics.)

I'll look at this separately; we also don't have test coverage of it.

>
>> @@ -2411,6 +2392,24 @@ take_while(P, [X | Xs], Start) :-
>>  take_while(P, Xs) = Start :-
>>      take_while(P, Xs, Start).
>>
>> +%---------------------------------------------------------------------------%
>> +
>> +drop(N, Xs, FinalXs) :-
>> +    ( if N > 0 then
>> +        Xs = [_ | Tail],
>> +        list.drop(N - 1, Tail, FinalXs)
>> +    else
>> +        N = 0,
>> +        FinalXs = Xs
>> +    ).
>> +
>> +det_drop(N, Xs, FinalXs) :-
>> +    ( if list.drop(N, Xs, FinalXsPrime) then
>> +        FinalXs = FinalXsPrime
>> +    else
>> +        unexpected($file, $pred, "index out of range")
>> +    ).
>> +
>>  drop_while(_, [], []).
>>  drop_while(P, [X | Xs], End) :-
>>      ( if P(X) then
>
> I think this block of code should stay where it was. The code is
> better separated based on whether it is index-based or higher-order,
> rather than whether it is take or drop, in particular because only the
> former deals with negative indexes. (So, for example, as part of this
> review I looked at all of the existing index-based code as a whole,
> but didn't need to look at the higher-order code. It was useful that
> the code I wanted to look at was grouped together.)

Done.

Julien.