[m-rev.] for review: reading multibyte integers from binary file streams

[Julien Fischer]

Hi Zoltan,

On Sun, 27 Jan 2019, Zoltan Somogyi wrote:

> On Sun, 20 Jan 2019 04:36:28 +0000 (UTC), Julien Fischer <jfischer at opturion.com> wrote:
>> I'm going to go ahead and commit this one now; I'll deal with any further
>> review comments separately.
>
> Sorry for the late review. I am concerned that your code may try to
> reverse the bytes even inside uint8s, which would fail because there is
> no function to perform that noop. Can you have a look at *my* review,
> which takes the form of the attached diff?

> diff --git a/library/io.m b/library/io.m
> index 098f694..96b84d1 100644
> --- a/library/io.m
> +++ b/library/io.m
> @@ -885,21 +885,22 @@
>  :- pred read_binary_uint8(io.binary_input_stream::in, io.result(uint8)::out,
>      io::di, io::uo) is det.
> 
> -    % The following predicates read multibyte integer values from the current
> -    % binary input stream or from the specified binary input stream.
> +    % The following predicates read multibyte integer values, either
> +    % from the current binary input stream, or from the specified
> +    % binary input stream.
>      %
>      % The names of these predicates have the form:
>      %
>      %    read_binary_<TYPE><SUFFIX>
>      %
>      % where <TYPE> is the name of one of the Mercury multibyte fixed size
> -    % integer types. <SUFFIX> is optional and specifies what order the
> -    % bytes in input stream that make up the multibyte integer occur
> -    % in. It may be one of:
> +    % integer types. The optional <SUFFIX> specifies the order in which
> +    % the bytes that make up the multibyte integer occur in input stream.
> +    % The suffix may be one of:
>      %
> -    %     no suffix - native byte order of the underlying platform.
> -    %     "_le"     - little endian byte order.
> -    %     "_be"     - big endian byte order.
> +    % "_le":    the bytes are in little endian byte order.
> +    % "_be":    the bytes are in big endian byte order.
> +    % none:     the bytes are in the byte order of the underlying platform.

I have no objection to your change of formatting here, except that it is
no longer consistent with that of the documentation of the
write_binary_<TYPE>_SUFFIX> predicates.  If this changes, then so should
that.

...

>  // Build an n-bit unsigned integer using the bytes stored in the array
> -// 'buffer'.  The order of the bytes in the buffer are given by 'byte_order'.
> +// 'buffer'. The order of the bytes in the buffer are given by 'byte_order'.
>  // The result is assigned to the lvalue 'value'
>  //
>  // We have two definitions of this macro, one for big-endian machines
> -// and one for little-endian machines.
> +// and one for little-endian machines. The special treatment of n == 8
> +// is due to the fact that the notion of ""reversing the bytes"" does not
> +// make sense for one byte integers.

While your change to handle N = 8 is harmless, it's also never going
to be used.  The result type used to handle uint8s is different.  I suggest
just documenting that MR_build_uintN is for use with N={16,32,64} directly
above the macro definition and leaving it at that.

>  //
>  #if defined(MR_BIG_ENDIAN)
>  #define ML_build_uintN(n, byte_order, buffer, value)                 \
>      do {                                                             \
> -        if (byte_order == ML_LITTLE_ENDIAN) {                        \
> +        if (((n) != 8) && ((byte_order) == ML_LITTLE_ENDIAN)) {      \
>              value = ML_REVERSE_BYTES_FUNC(n)(                        \
> -                *((ML_N_BIT_INT_T(n) *) buffer));                    \
> +                *((ML_N_BIT_UINT_T(n) *) buffer));                   \
>          } else {                                                     \
> -            value = *((ML_N_BIT_INT_T(n) *) buffer);                 \
> +            value = *((ML_N_BIT_UINT_T(n) *) buffer);                \
>          }                                                            \
>      } while (0)
>  #else
>  #define ML_build_uintN(n, byte_order, buffer, value)                 \
>      do {                                                             \
> -        if (byte_order == ML_LITTLE_ENDIAN) {                        \
> -            value = *((ML_N_BIT_INT_T(n) *) buffer);                 \
> +        if (((n) == 8) || ((byte_order) == ML_LITTLE_ENDIAN)) {      \
> +            value = *((ML_N_BIT_UINT_T(n) *) buffer);                \
>          } else {                                                     \
>              value = ML_REVERSE_BYTES_FUNC(n)(                        \
> -                *((ML_N_BIT_INT_T(n) *) buffer));                    \
> +                *((ML_N_BIT_UINT_T(n) *) buffer));                   \
>          }                                                            \
>      } while (0)
>  #endif

The rest looks fine.

Julien.