[m-rev.] for review: writing 16- and 32-bit integers to binary file streams

[Zoltan Somogyi]

On Mon, 25 Sep 2017 01:04:15 -0400 (EDT), Julien Fischer <jfischer at opturion.com> wrote:
> Also, does anyone have any better suggestion as to the name of the new 
> runtime header file?

I think mercury_int.h is ok.

> runtime/mercury_int.h:
>      New header intended to hold things related to the implementation
>      of the integer types in C grades.  Currently it contains the
>      macros defining the byte reverse operations for 16- and 32-bit
>      types.   (It will eventually containing macros for boxing 64-bit
>      integer types on 32-bit platforms as well.)

s/containing/contain/

> diff --git a/library/io.m b/library/io.m
> index a3320d4..aecd8d2 100644
> --- a/library/io.m
> +++ b/library/io.m
> @@ -1026,6 +1026,94 @@
>   :- pred write_binary_uint8(io.binary_output_stream::in, uint8::in,
>       io::di, io::uo) is det.
> 
> +    % Writes a signed 16-bit integer to the current binary output stream or
> +    % to the specified binary output stream in the native byte order of the
> +    % underlying platform.
> +    %
> +:- pred write_binary_int16(int16::in, io::di, io::uo) is det.
> +:- pred write_binary_int16(io.binary_output_stream::in, int16::in,
> +    io::di, io::uo) is det.
> +
> +    % Writes a signed 16-bit integer to the current binary output stream or
> +    % to the specified binary output stream in little endian byte order.
> +    %
> +:- pred write_binary_int16_le(int16::in, io::di, io::uo) is det.
> +:- pred write_binary_int16_le(io.binary_output_stream::in, int16::in,
> +    io::di, io::uo) is det.
> +
> +    % Writes a signed 16-bit integer to the current binary output stream or
> +    % to the specified binary output stream in big endian byte order.
> +    %
> +:- pred write_binary_int16_be(int16::in, io::di, io::uo) is det.
> +:- pred write_binary_int16_be(io.binary_output_stream::in, int16::in,
> +    io::di, io::uo) is det.

Basically, you have almost the same code here repeated many times here.
There are four dimensions of variability:

- 16 vs 32 bit
- signed vs unsigned
- endianness (native, little, big)
- explicit vs implicit stream

In such cases, I would write just ONE comment for ALL these predicate declarations,
explicitly giving the naming scheme for the first three dimensions (the fourth
is in the arity, not the names).

> +write_binary_int16(Int16, !IO) :-
> +    UInt16 = uint16.cast_from_int16(Int16),
> +    write_binary_uint16(UInt16, !IO).
> +
> +write_binary_int16_le(Int16, !IO) :-
> +    UInt16 = uint16.cast_from_int16(Int16),
> +    write_binary_uint16_le(UInt16, !IO).
> +
> +write_binary_int16_be(Int16, !IO) :-
> +    UInt16 = uint16.cast_from_int16(Int16),
> +    write_binary_uint16_be(UInt16, !IO).
> +
> +write_binary_uint16(UInt16, !IO) :-
> +    binary_output_stream(Stream, !IO),
> +    write_binary_uint16(Stream, UInt16, !IO).
> +
> +write_binary_uint16_le(UInt16, !IO) :-
> +    binary_output_stream(Stream, !IO),
> +    write_binary_uint16_le(Stream, UInt16, !IO).
> +
> +write_binary_uint16_be(UInt16, !IO) :-
> +    binary_output_stream(Stream, !IO),
> +    write_binary_uint16_be(Stream, UInt16, !IO).

I find such code is easier to read if you have short line dividers
dividing the above into blocks of predicates. In this case, you may want
to use minimal-length dividers creating blocks of three clauses
(native, _le and _be versions), slightly longer dividers for int vs uint,
and longer still dividers for 16 vs 32 bit.

> +write_binary_int16(Stream, Int16, !IO) :-
> +    UInt16 = uint16.cast_from_int16(Int16),
> +    write_binary_uint16(Stream, UInt16, !IO).
> +
> +write_binary_uint16(binary_output_stream(Stream), UInt16, !IO) :-
> +    do_write_binary_uint16(Stream, UInt16, Error, !IO),
> +    throw_on_output_error(Error, !IO).
> +
> +:- pred do_write_binary_uint16(stream::in, uint16::in, system_error::out,
> +    io::di, io::uo) is det.
> +
> +:- pragma foreign_proc("C",
> +    do_write_binary_uint16(Stream::in, U16::in, Error::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, thread_safe, tabled_for_io],
> +"
> +    if (MR_WRITE(*Stream, (unsigned char *)(&U16), 2) != 2) {
> +        Error = errno;
> +    } else {
> +        Error = 0;
> +    }
> +").
> +
> +:- pragma foreign_proc("Java",
> +    do_write_binary_uint16(Stream::in, U16::in, Error::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, thread_safe, tabled_for_io],
> +"
> +    try {
> +        java.nio.ByteBuffer buffer = java.nio.ByteBuffer.allocate(2);
> +        buffer.order(java.nio.ByteOrder.nativeOrder());
> +        buffer.putShort(U16);
> +        ((io.MR_BinaryOutputFile) Stream).write(buffer.array(), 0, 2);
> +        Error = null;
> +    } catch (java.io.IOException e) {
> +        Error = e;
> +    }
> +").
> +
> +:- pragma foreign_proc("C#",
> +    do_write_binary_uint16(Stream::in, U16::in, Error::out, _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure, thread_safe, tabled_for_io],
> +"
> +    byte[] bytes = BitConverter.GetBytes(U16);
> +    try {
> +        Stream.stream.Write(bytes, 0, 2);
> +        Error = null;
> +    } catch (System.Exception e) {
> +        Error = e;
> +    }
> +").

Dividers would help here as well.

I cannot check the correctness of the Java and C# foreign_procs.

> +#if defined(MR_GNUC) || defined(MR_CLANG)
> +  #define MR_uint16_reverse_bytes(U) __builtin_bswap16((U))
> +#else
> +  #define MR_uint16_reverse_bytes(U) ((U >> 8) | (U << 8))
> +#endif
> +
> +#if defined(MR_GNUC) || defined(MR_CLANG)
> +  #define MR_uint32_reverse_bytes(U) __builtin_bswap32((U))
> +#else
> +  #define MR_uint32_reverse_bytes(U) ((U & UINT32_C(0x000000ff)) << 24 | \
> +                                      (U & UINT32_C(0x0000ff00)) << 8  | \
> +                                      (U & UINT32_C(0x00ff0000)) >> 8  | \
> +                                      (U & UINT32_C(0xff000000)) >> 24 )

I thought after the discussion on the mailing list, you were going to change
the 16 bit code to use explicit bit masks like the 32 bit version.

Otherwise, the diff is fine (the part I could check, that is).

Zoltan.