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

[Julien Fischer]

Hi Zoltan,

On Mon, 25 Sep 2017, Zoltan Somogyi wrote:

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

Fixed.

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

How's this?

     The following predicates write multibyte integer values to the current
     binary output stream or to the specified binary output stream.

     These names of these predicates have the form:

        write_binary_<TYPE><SUFFIX>

     where <TYPE> is the name of one of the Mercury multibyte fixed size
     integer types. <SUFFIX> is optional and specifies the byte order in
     which the integer value is written to the stream.  It may be one of:

        no suffix - native byte order of the underlying platform.
        "_le"     - little endian byte order.
        "_be"     - big endian byte order.

(That should also cover the 64-bit variants when they're added.)

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

Added.

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

Added as well.

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

Yes, but I only replied to Peter's review of my other change *after* I
send this mail ;-)   I've added the bitmasks in.

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

Thanks for the review.

Julien.