[m-rev.] for review: add builtin 64-bit integer types -- part 2
Julien Fischer
jfischer at opturion.com
Sat Feb 3 02:28:55 AEDT 2018
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.
More information about the reviews
mailing list