[m-rev.] for review: start passing output streams explicitly

[Peter Wang]

On Fri, 13 Nov 2020 14:05:23 +1100 "Zoltan Somogyi" <zoltan.somogyi at runbox.com> wrote:
> I apologize for the huge size of this diff (almost 40,000 lines), but
> it is as small(!) as I could make it given the circumstances. And
> almost all of it is trivial. I would appreciate a review, but if noone
> volunteers to check even the few modules explicitly mentioned
> at the top of the log message, I could commit the diff and fix
> any problems that arose later.
> 
> I have bootchecked near-final versions of this diff in several grades:
> 
> in asm_fast.gc
> in asm_fast.gc.debug.stseg
> in asm_fast.gc.profdeep
> in hlc.gc
> in hlc.gc with -O5 --intermodule
> in csharp
> in java
> 
> and I intend to repeat these bootchecks with the final version
> (after addressing any review comments) before commit.
> (People are welcome to request additional ways for me to
> bootcheck the diff as well.)

Hmm, perhaps somewhere (not sure where) you could set the current output
stream to append to a temporary file, and see if anything gets written
to it.

> ZZZ
> Write to explicitly named streams in many modules.

Remember to delete ZZZ.

> This typeclass, output(U), has two instances: output(io), and output(string),
> so you could output either to the current output stream, or to a string.
> To allow the specification of the destination stream in the first case,
> this diff changes the typeclass to output(S, U) with a functional dependency
> from U to S, with the two instances being output(io.text_output_stream, io)
> and output(unit, string). (The unit arg is ignored in the second case.)

A bespoke type might be better, but it doesn't matter.

> 
> There is a complication with the output typeclass method, add_list, that
> outputs a list of items. The complication is that each item is output
> by a predicate supplied by the caller, but the separator between the items
> (usually a comma) is output by add_list itself. We don't want to give
> callers of this method the opportunity to screw up by specifying two
> different output streams for these two purposes,

That should be an unlikely mistake to make, I think. The more likely
mistake, that I have made before, is to pass a higher order predicate
that writes to the current output stream instead of an explicit stream.
That should be caught by --warn-implicit-stream-calls.

> so we want (a) the caller
> to tell add_list where to put the separators, and then (b) for add_list,
> not its caller, tell the user-supplied predicate what stream to write to.
> This works only if the stream argument is just before the di,uo pair
> of I/O state arguments, which differs from our usual practice of passing
> the stream at or near the left edge of the argument list, not near the right.
> The result of this complication is that two categories of predicates that
> are and are not used to print items in a list differ in where they put
> the stream in their argument lists. This makes it easy to pass the stream
> in the wrong argument position if you call a predicate without looking up
> its signature, and may require *changing* the argument order when a
> predicate is used to print an item in a list for the first time.
> A complete switch over to always passing the stream just before !IO
> would fix this inconsistency, but is far to big a change to make all at once.

There would be an inconsistency with the io module predicates anyway.
Aside from the add_list problem, I think the stream makes more sense
near the start of the argument list as it works well with list.foldl.
The inconsistency for the relatively few calls to add_list should be
acceptable.

> compiler/parse_tree_out_info.m:
> compiler/prog_out.m:
> compiler/c_util.m:
> compiler/name_mangle.m:
> compiler/file_util.m:

I looked at these files and glanced through some of the rest.
Seems fine.

> diff --git a/compiler/c_util.m b/compiler/c_util.m
> index ba67bb5f3..7a4a63681 100644
> --- a/compiler/c_util.m
> +++ b/compiler/c_util.m
...
>      % Output #pragma pack directives to change the packing alignment value
>      % for MSVC. See MR_Float_Aligned in mercury_float.h.
> -    %
> -:- pred output_pragma_pack_push(io::di, io::uo) is det.
>  
> -:- pred output_pragma_pack_pop(io::di, io::uo) is det.
> +:- pred output_pragma_pack_push(io.text_output_stream::in,
> +    io::di, io::uo) is det.
> +:- pred output_pragma_pack_push_current_stream(io::di, io::uo) is det.
> +
> +:- pred output_pragma_pack_pop(io.text_output_stream::in,
> +    io::di, io::uo) is det.
> +:- pred output_pragma_pack_pop_current_stream(io::di, io::uo) is det.

cur_stream to match other predicates?

> diff --git a/compiler/file_util.m b/compiler/file_util.m
> index f11e52e52..acecb72f4 100644
> --- a/compiler/file_util.m
> +++ b/compiler/file_util.m
...
> +output_to_file_stream(Globals, FileName, Action0, Succeeded, !IO) :-
> +    globals.lookup_bool_option(Globals, verbose, Verbose),
> +    globals.lookup_bool_option(Globals, statistics, Stats),
> +    maybe_write_string(Verbose, "% Writing to file `", !IO),
> +    maybe_write_string(Verbose, FileName, !IO),
> +    maybe_write_string(Verbose, "'...\n", !IO),
> +    maybe_flush_output(Verbose, !IO),
> +    io.open_output(FileName, Res, !IO),
> +    (
> +        Res = ok(FileStream),
> +        Action =
> +            ( pred(E::out, S0::di, S::uo) is det :-
> +                call(Action0, FileStream, E, S0,S)

space

> diff --git a/compiler/parse_tree_out_info.m b/compiler/parse_tree_out_info.m
> index 27fead93e..e8a2519ca 100644
> --- a/compiler/parse_tree_out_info.m
> +++ b/compiler/parse_tree_out_info.m
...
> +    % In the (output stream, I/O state) instance, the add_list predicate
> +    % (a) prints the separator to the specified stream S, and (b)
> +    % tell the predicate that prints each item to print that item to stream S.
> +    pred add_list(pred(T, S, U, U)::in(pred(in, in, di, uo) is det),
> +        string::in, list(T)::in, S::in, U::di, U::uo) is det

I suggest

    The add_list predicate calls a predicate to write each element of
    the list to the specified stream, printing a separator between each
    pair of elements.

> diff --git a/compiler/parse_tree_out_inst.m b/compiler/parse_tree_out_inst.m
> index 4fbebed20..9cc6be9f3 100644
> --- a/compiler/parse_tree_out_inst.m
> +++ b/compiler/parse_tree_out_inst.m
...
> -mercury_output_mode(Lang, InstVarSet, Mode, !IO) :-
> -    mercury_format_mode(Lang, InstVarSet, Mode, !IO).
> +mercury_output_mode(Streram, Lang, InstVarSet, Mode, !IO) :-
> +    mercury_format_mode(Lang, InstVarSet, Mode, Streram, !IO).

Streram

> diff --git a/compiler/parse_tree_out_pragma.m b/compiler/parse_tree_out_pragma.m
> index b31c20c7e..2ceab89e1 100644
> --- a/compiler/parse_tree_out_pragma.m
> +++ b/compiler/parse_tree_out_pragma.m
> @@ -1563,56 +1586,52 @@ write_vars_and_types(VarSet, TypeVarSet, HeadVars, HeadVarTypes, !IO) :-
>  % Output a require_feature_set pragma.
>  %
>  
> -:- pred mercury_output_pragma_require_feature_set(
> -    pragma_info_require_feature_set::in, U::di, U::uo) is det <= output(U).
> +:- pred mercury_format_pragma_require_feature_set(
> +    pragma_info_require_feature_set::in, S::in,
> +    U::di, U::uo) is det <= output(S, U).
>  
> -mercury_output_pragma_require_feature_set(RFSInfo, !U) :-
> +mercury_format_pragma_require_feature_set(RFSInfo, S, !U) :-
>      RFSInfo = pragma_info_require_feature_set(Features0),
>      Features = set.to_sorted_list(Features0),
> -    add_string(":- pragma require_feature_set(", !U),
> -    add_list(Features, ",", mercury_format_required_feature, !U),
> -    add_string(").\n", !U).
> -
> -:- pred mercury_format_required_feature(required_feature::in, U::di, U::uo)
> -    is det <= output(U).
> -
> -mercury_format_required_feature(reqf_concurrency, !U) :-
> -    add_string("concurrency", !U).
> -mercury_format_required_feature(reqf_single_prec_float, !U) :-
> -    add_string("single_prec_float", !U).
> -mercury_format_required_feature(reqf_double_prec_float, !U) :-
> -    add_string("double_prec_float", !U).
> -mercury_format_required_feature(reqf_memo, !U) :-
> -    add_string("memo", !U).
> -mercury_format_required_feature(reqf_parallel_conj, !U) :-
> -    add_string("parallel_conj", !U).
> -mercury_format_required_feature(reqf_trailing, !U) :-
> -    add_string("trailing", !U).
> -mercury_format_required_feature(reqf_strict_sequential, !U) :-
> -    add_string("strict_sequential", !U).
> -mercury_format_required_feature(reqf_conservative_gc, !U) :-
> -    add_string("conservative_gc", !U).
> +    add_string(":- pragma require_feature_set(", S, !U),
> +    add_list(mercury_format_required_feature, ",", Features, S, !U),

(the old code was also missing a space after the comma)

Peter