[m-rev.] for review: start passing output streams explicitly
novalazy at gmail.com
Fri Nov 13 18:07:28 AEDT 2020
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
> 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
I looked at these files and glanced through some of the rest.
> 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)
> 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
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).
> 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)
More information about the reviews