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

[Zoltan Somogyi]

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 ..."

Making the shift amount a uint works. There is no advantage in making it
the same type as the value being shifted, and even uint8 is too big for
the intended range.

> +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/

> +:- 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".

> +    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.

> +*** 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?

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.

The diff is otherwise fine.

Zoltan.