[m-rev.] for review: add bitwise rotations for uint32s

[Julien Fischer]

Hi Zoltan,

On Sun, 28 Mar 2021, Zoltan Somogyi wrote:

> 2021-03-28 02:22 GMT+11:00 "Julien Fischer" <jfischer at opturion.com>:
>> +    % unchecked_rotate_left(U, D) = N:
>> +    %
>> +    % N is the value obtained by rotating the binary representation of U
>> +    % left by the lowest 5 bits of D.
>> +    %
>> +:- func unchecked_rotate_left(uint32, uint) = uint32.
>
> I would reword the end of the comment as "by an amount given by the lowest ..."

Done.

,,,

>> +rotate_left(X, N) =
>> +    ( if N < 32u then
>> +        unchecked_rotate_left(X, N)
>> +    else
>> +        func_error($pred, "second argument exceeds 31 bits")
>> +    ).
>
> s/second argument/shift amount/

Perhaps "rotate amount" rather than "shift amount"?

>> +:- pragma foreign_proc("C",
>> +    unchecked_rotate_left(X::in, N::in) = (Result::out),
>> +    [will_not_call_mercury, promise_pure, thread_safe],
>> +"
>> +    N &= 31;
>> +    // This implementation is from https://blog.regehr.org/archives/1063.
>> +    // It is intended to avoid undefined behaviour in C and be recognisable by
>> +    // C compilers as a rotate operation. (On architectures that have a rotate
>> +    // instruction
>
> Add a comma after "instruction".

Done.

>> +    N &= 31;
>> +    Result = (X << (int) N) | (X >> (int) (-N & 31));
>> +").
>> +
>> +:- pragma foreign_proc("Java",
>> +    unchecked_rotate_left(X::in, N::in) = (Result::out),
>> +    [will_not_call_mercury, promise_pure, thread_safe],
>> +"
>> +    Result = java.lang.Integer.rotateLeft(X, N);
>> +").
>
> The Java version has no N &= 31. Why?
>
> And the same for the _right versions.

It's unecessary.  The rotateLeft and rotateRight methods in Java
do that anyway.

>> +*** Test 'rotate_left' ***
>
> I would change the test case code to print the results of unchecked_rotate_dir for a given
> pair of input args just after rotate_dir, so that these two lines would be next to each other
> (and lined up to make a visual diff trivial).
>
>> +                      rotate_left(00000000000000000000000000000001, 0) = 00000000000000000000000000000001
>> +unchecked_rotate_left(00000000000000000000000000000001, 0) = 00000000000000000000000000000001
>
>> +:- func to_binary_string_lz(uint32::in) = (string::uo) is det.
>
> What is the _lz for?

lz == leading zeros

> We have string.int_to_base_string in the standard library. This looks like a case for
> adding unsigned and size-specific versions of it as well. In fact, any kind of conversion
> to string that exists for any integer type should exist for all ten integer types.

True, I'll look into adding the missing ones.

> The diff is otherwise fine.

Thanks for the review.

Julien.