[m-rev.] for review: io.primitives_{read,write}.m

[Julien Fischer]

Hi Zoltan,

On Sat, 12 Mar 2022, Zoltan Somogyi wrote:

> Carve io.primitives_{read,write}.m out of io.m.
> 
> library/io.m:
> library/io.primitives_read.m:
> library/io.primitives_write.m:
>     As above. The two new private submodules contain the implementations
>     (helper predicates) of the operations that respectively read and write
>     values of primitive types.
> 
> library/MODULES_UNDOC:
> library/library.m:
>     List the new submodules as undocumented.

...

> diff --git a/library/io.primitives_read.m b/library/io.primitives_read.m
> index e69de29bb..7e915b97d 100644
> --- a/library/io.primitives_read.m
> +++ b/library/io.primitives_read.m
> @@ -0,0 +1,871 @@
> +%---------------------------------------------------------------------------%
> +% vim: ft=mercury ts=4 sw=4 et
> +%---------------------------------------------------------------------------%
> +% Copyright (C) 1993-2012 The University of Melbourne.
> +% Copyright (C) 2013-2022 The Mercury team.
> +% This file is distributed under the terms specified in COPYING.LIB.
> +%---------------------------------------------------------------------------%
> +%
> +% File: io.primitives_read.m.
> +%
> +% This file handles the details of reading in values of primitive types.
> +%
> +% We communicate results from foreign_procs as separate simple arguments
> +% so the C/Java/etc code does not depend on how Mercury stores its
> +% discriminated union data types. It also avoids memory allocation in
> +% inner loops.
> +%
> +%---------------------------------------------------------------------------%
> +%---------------------------------------------------------------------------%
> +
> +:- module io.primitives_read.
> +:- interface.
> +
> +:- import_module char.
> +
> +    % We communicate results from foreign_procs as separate simple arguments
> +    % so the C/Java/etc code does not depend on how Mercury stores its
> +    % discriminated union data types. It also avoids memory allocation in
> +    % inner loops.

That comment just repeats the one in the head of the file.

> +%---------------------------------------------------------------------------%
> +:- implementation.
> +%---------------------------------------------------------------------------%

This code is lacking a C foreign_decl that #includes stdint.h, inttypes.h
mercury_int.h.  (Conversely, those headers may not be #included in io.m's
C foreign_decl block.)

...

> +:- pragma foreign_proc("C#",
> +    read_char_code_2(File::in, ResultCode::out, Char::out, Error::out,
> +        _IO0::di, _IO::uo),
> +    [will_not_call_mercury, promise_pure],
> +"
> +    io.MR_MercuryFileStruct mf = File;
> +    try {
> +        int c = io.mercury_getc(mf);
> +        if (c == -1) {
> +            ResultCode = io.ML_RESULT_CODE_EOF;
> +            Char = 0;
> +        } else {
> +            ResultCode = io.ML_RESULT_CODE_OK;
> +            Char = c;
> +        }
> +        Error = null;
> +    } catch (System.Exception e) {
> +        ResultCode = io.ML_RESULT_CODE_ERROR;
> +        Char = 0;
> +        Error = e;
> +    }
> +").

As a generaly point, I would fully qualify things in Java and C# foreign_procs.
The required import directives may not be visible when they are inlined via
intermodule-optimization and we cannot also guarantee that user defined names
are not going to clash with partially qualified names in our foreign_procs.
(That last was is fairly unlikely in most cases.)

This is likely an existing issue with this code, but wasn't really a problem
since when they lived in io.m these foreign_procs were never really inlined
across module boundaries anyway.

...

> diff --git a/library/io.primitives_write.m b/library/io.primitives_write.m
> index e69de29bb..bfdc50e0c 100644
> --- a/library/io.primitives_write.m
> +++ b/library/io.primitives_write.m

...

> +:- pred do_write_binary_uint64(stream::in, uint64::in, system_error::out,
> +    io::di, io::uo) is det.
> +
> +:- pred do_write_binary_uint64_le(stream::in, uint64::in, system_error::out,
> +    io::di, io::uo) is det.
> +
> +:- pred do_write_binary_uint64_be(stream::in, uint64::in, system_error::out,
> +    io::di, io::uo) is det.
> +
> +%---------------------------------------------------------------------------%
> +
> +:- implementation.
> +
> +% ZZZ :- import_module array.
> +
> +% ZZZ :- pragma foreign_import_module("C#", array).

Why are those there?

Again, there is a lack of #includes for stdint.h, inttypes.h etc here.

That looks fine otherwise.

Julien.