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

[Mark Brown]

Hi Julien,

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. Or perhaps just leave out the quotes entirely, as more recent
parts of the documentation tend to do?

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

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

The rest looks fine.

Mark