[m-rev.] for review: add builtin 64-bit integer types -- part 2

[Julien Fischer]

On Sat, 3 Feb 2018, Zoltan Somogyi wrote:

> On Thu, 1 Feb 2018 21:03:12 -0500 (EST), Julien Fischer <jfischer at opturion.com> wrote:
>> Have you had an opportunity to look at the library changes
>> yet?
>
> Yes, but I forgot :-(
>
> Thanks for reminding me. My review of the library is attached.
>
> As for the test cases, you are right, adding the C code generating
> the "guaranteed correct" output in the test cases themselves won't work.
>
> For each Mercury test case, we should have C program that does the same
> computations and outputs the same results in the same format,
> which we would update in sync with the Mercury test case,
> and execute once per update manually to generate the .exp file.
> Would that work for you?

Sure, I'll do that separately though.
(I have been intending to do something similar for arbitrary precision
integers for a long time.)

> diff --git a/library/int64.m b/library/int64.m
> index 4876351..6c15948 100644
> --- a/library/int64.m
> +++ b/library/int64.m
> @@ -17,5 +17,447 @@
>  :- module int64.
>  :- interface.
> 
> -:- type placeholder_int64 ---> placeholder_int64.
> +:- import_module pretty_printer.
> 
> +%---------------------------------------------------------------------------%
> +
> +    % from_int(I) = I64:
> +    % Convert an int to an int64.
> +    %
> +:- func from_int(int) = int64.
> +
> +:- func cast_to_int(int64) = int.
> +
> +:- func cast_from_uint64(uint64) = int64.
> +
> +%---------------------------------------------------------------------------%
> +
> +    % Less than.
> 
> Will there be from_bytes_{be,le} here eventually, for consistency?

Yes, all the integer modules will eventually provide the same set of operations
apart from a few differences between signed and unsigned types, and some of the
conversion operations.

Other modules that provide operations on int types (e.g. the io module) will be
treated similarly.

> +    % unchecked_abs(X) retursn the absolute value of X, except that if the
> +    % result is undefined if X = int64.min_int64.
> +    %
> +:- func unchecked_abs(int64) = int64.
> 
> s/that if the/that the/
> 
> Here *and* in the other similar modules.

Fixed.

> +    % nabs(X) retursn the negative absolute value of X.
> +    % Unlike abs/1 this function is defined for X = int64.min_int64.
> +    %
> +:- func nabs(int64) = int64.
> 
> I would s/negative absolute value/negative of the absolute value/,
> again both here *and* in the other similar modules.

Done.

> +    % behaviour is undefined if Y is not in [0, 64).
> +    % It will typically be be implemented more efficiently than X << Y.
> +    %
> +:- func unchecked_left_shift(int64::in, int::in) = (int64::uo) is det.
> 
> be be

Fixed.

....

> +    % Bitwise complement.
> +    %
> +:- func \ (int64::in) = (int64::uo) is det.
> 
> Will there be reverse_bytes?

Eventually.

> In general, I would aim for the modules of the builtin int types
> to look all the same (have the same predicates in the same order)
> as far as possible, both to make maintenance easier and to avoid
> surprises for users.

Agreed.

> +:- func min_int64 = int64.
> +
> +:- func max_int64 = int64.
> +
> +    % Convert a int64 to a pretty_printer.doc for formatting.
> +    %
> +:- func int64_to_doc(int64) = pretty_printer.doc.
> 
> I would s/a int64/an int64/ here, and do similarly in the other
> int modules.

Done.  (I've also added uint64_to_doc/1 which was missing.)

> +%---------------------------------------------------------------------------%
> +
> +X div Y = Div :-
> +    Trunc = X // Y,
> +    ( if
> +        ( X >= from_int(0), Y >= from_int(0)
> +        ; X < from_int(0), Y < from_int(0)
> +        ; X rem Y = from_int(0)
> +        )
> +    then
> +        Div = Trunc
> +    else
> +        Div = Trunc - from_int(1)
> +    ).
> 
> I would mark this and below with an XXX INT64 to remind myself
> to come back and replace those from_int(0)s later.

Not really worth doing for the int64 and uint64 modules.  (It will be pretty
much the first thing done after this change bootstraps in any case.)

> diff --git a/library/integer.m b/library/integer.m
> index c1b08b4..33ed394 100644
> --- a/library/integer.m
> +++ b/library/integer.m
> @@ -1517,6 +1559,74 @@ det_to_uint32(Integer) = UInt32 :-
>
>  %---------------------------------------------------------------------------%
> 
> +to_int64(Integer, Int64) :-
> +    Integer = i(Sign, Digits),
> +    compare(SignRes, Sign, 0),
> +    (
> +        SignRes = (<),
> +        Integer >= integer_min_int64,
> +        Int64 = int64_list(Digits, from_int(0))
> +    ;
> +        SignRes = (=),
> +        Int64 = from_int(0)
> +    ;
> +        SignRes = (>),
> +        Integer =< integer_max_int64,
> +        Int64 = int64_list(Digits, from_int(0))
> +    ).
> +
> +:- func integer_min_int64 = integer.
> +
> +integer_min_int64 = i(-5, [-128, 0, 0, 0, 0]).
> +
> +:- func integer_max_int64 = integer.
> +
> +integer_max_int64 = i(5, [127, 16383, 16383, 16383, 16383]).
> 
> I would document both integer constants by adding a comment
> to the effect of 128 * 2^(14*4) = 2^7 * 2^(56) = 2^63,
> with a -1 for the second.

Done.

> +integer_max_uint64 = i(5, [255, 16383, 16383, 16383, 16383]).
> 
> And here.

Done.

> @@ -1832,6 +1942,75 @@ uint32_to_digits_2(U, Tail) = Result :-
>              i(Length + 1, [cast_to_int(Mod) | Digits]))
>      ).
> 
> +from_int64(I64) = Integer :-
> +    ( if
> +        I64 = from_int(0)
> +    then
> +        Integer = integer.zero
> +    else if
> +        I64 > from_int(0),
> +        I64 < from_int(base)
> +    then
> +        Integer = i(1, [cast_to_int(I64)])
> +    else if
> +        I64 < from_int(0),
> +        I64 > from_int(-base)
> +    then
> +        Integer = i(-1, [cast_to_int(I64)])
> +    else
> +        ( if I64 = int64.min_int64 then
> +            Integer = integer.from_int64(I64 + from_int(1)) - integer.one
> +        else
> +            Magnitude = pos_int64_to_digits(int64.abs(I64)),
> +            ( if I64 < int64.from_int(0) then
> +                Integer = big_neg(Magnitude)
> +            else
> +                Integer = Magnitude
> +            )
> +        )
> +    ).
> 
> Please add a comment saying that min_int64 needs special treatment
> because abs won't work on it.

Done

> Why the second level of nesting?

I think it was rather late in the evening when I wrote it -- fixed.

> diff --git a/library/uint64.m b/library/uint64.m
> index be4664d..2d3d120 100644
> --- a/library/uint64.m
> +++ b/library/uint64.m
> +    %
> +:- func (uint64::in) << (int::in) = (uint64::uo) is det.
> +
> +    % unchecked_left_shift(X, Y) is the same as X << Y except that the
> +    % behaviour is undefined if Y is not in [0, 64).
> +    % It will typically be be implemented more efficiently than X << Y.
> +    %
> +:- func unchecked_left_shift(uint64::in, int::in) = (uint64::uo) is det.
> 
> "be be" is here, so please check my other comments for int64.m
> for applicability here as well.

Done in all affected modules.

> +det_from_int(I) = U :-
> +    ( if from_int(I, U0)
> +    then U = U0
> +    else error("uint64.det_from_int: cannot convert int to uint64")
> +    ).
> 
> Please don't follow Ralph's formatting style for if-then-elses.

I've changed it to the usual one.

> The rest of the library looks fine.

Thanks for the review.

Julien.