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

[Julien Fischer]

Hi Zoltan,

On Tue, 8 Oct 2019, Zoltan Somogyi wrote:

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

No, I was waiting for it to be reviewed ;-)

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

I'll do that in a future change.

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

I'll take a look.

>
>>       % 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?

No, the final versions will use the unsuffixed names but with int64
offset arguments.  (It will take us a few releases to get there though.)

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

The problem is that the existing version is not necessarily 32-bit
already -- what it is varies depending on what size a Mercury int is and
whether we are on Windows or not (i.e. it is already 64-bit on many
platforms but Mercury programmers cannot rely on that).

The right thing to do is transition users to a version that always uses
64-bit offsets.  All the underlying platforms provide that anyway.

I don't think there is anything to be gained by adding _32 version.
We can make the existing unsuffixed version safer when using 32-bit
ints, but it's fundamentally pretty broken for files > 2GB on some
backends (e.g. Java).

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

Ok, changed.

Julien.