[m-rev.] for review: add 64-bit versions of seek and offset on binary streams

[Zoltan Somogyi]

On Fri, 4 Oct 2019 17:09:19 +1000 (AEST), Julien Fischer <jfischer at opturion.com> wrote:
> I'll update the NEWS file separately after I've committed other related
> changes -- I want to commit this now as it is required by those changes.

You have not pushed that commit yet.

> My intention is that we will eventually mark the existing versions of
> seek and offset as obsolete and then after a time change their arguments
> to be int64s (i.e. in the long run we won't need to have two versions of
> seek and offset).

Agreed.

> I would like opinions on what we should do on platforms with 32-bit ints
> where binary_input_stream_offset, for example, returns a value that
> doesn't fit in a 32-bit int; currently we just truncate the returned
> offset, I think we should change it throw an exception if that happens.

Agree: throwing an exception is the right thing to do.

> library.io.m:
>       Add the predicates seek64_binary_{input,output}/5 and
>       binary_{input,output}_stream_offset64/4.
> 
>       Shift the input stream versions of the above operations into the correct
>       section; for some reason they were listed with the predicates that
>       operate on binary *output* streams.

Part of the problem is that io.m is so old and jumbled that there is no clearly
marked overarching section structure. It is one of the few library modules
for which the inconsistent order warning is disabled, because it would generate
warnings that halt at warn would turn into errors. I was planning to fix this
once you had finished your changes, but maybe you would like to fix it
earlier, since it could make your changes easier. My notes on the what I think
a good section structure would be are attached.

>       % Returns the offset (in bytes) into the specified binary output stream.
>       %
>   :- pred binary_output_stream_offset(io.binary_output_stream::in, int::out,
>       io::di, io::uo) is det.
> 
> +    % As above, but the offset is always a 64-bit value.
> +    %
> +:- pred binary_output_stream_offset64(io.binary_output_stream::in, int64::out,
> +    io::di, io::uo) is det.

Do you intend for the final predicates names to have the 64 suffix?
I would have thought that we could have three versions now:
plain, _32 suffix, and _64 suffix. For now, the plain version would redirect
to the _32 version, with an obsolete pragma in favor of the _32 version,
but would then be made identical to the_64 version, with a pragma obsolete on the
_64 version in favor of the plain version.

>   %---------------------------------------------------------------------------%
>   %
>   % Binary input stream predicates.
> @@ -1438,6 +1434,30 @@
>   :- pred binary_input_stream_name(io.binary_input_stream::in, string::out,
>       io::di, io::uo) is det.
> 
> +    % Seek to an offset relative to Whence (documented above)
> +    % on a specified binary input stream. Attempting to seek on a pipe
> +    % or tty results in implementation dependent behaviour.
> +    %
> +    % A successful seek undoes any effects of putback_byte on the stream.
> +    %
> +:- pred seek_binary_input(io.binary_input_stream::in, io.whence::in,
> +    int::in, io::di, io::uo) is det.
> +
> +    % As above, but the offset is always a 64-bit value.
> +    %
> +:- pred seek64_binary_input(io.binary_input_stream::in, io.whence::in,
> +    int64::in, io::di, io::uo) is det.

I don't like the idea of a 64 in the *middle* of the predicate name; it should be
a suffix.

> @@ -9359,15 +9383,25 @@ whence_to_int(end, 2).
> 
>   seek_binary_input(binary_input_stream(Stream), Whence, Offset, !IO) :-
>       whence_to_int(Whence, Flag),
> -    seek_binary_2(Stream, Flag, Offset, Error, !IO),
> +    seek_binary_2(Stream, Flag, int64.from_int(Offset), Error, !IO),
>       throw_on_error(Error, "error seeking in file: ", !IO).

If you added two auxiliary functions to convert each way between 32 and 64 bit offsets,
you would have only one place to change on changeover.

The rest of the diff looks fine.

Zoltan.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: Log.io
Type: application/octet-stream
Size: 834 bytes
Desc: not available
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20191008/fdd8aed8/attachment.obj>