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

Zoltan Somogyi zoltan.somogyi at runbox.com
Sat Nov 14 01:46:55 AEDT 2020


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

Unfortunately, I don't think that would catch every problem, because
the compiler can, and in some circumstances will, redirect the current
output to go somewhere later. The emptiness of such a "dumpster file"
thus does not imply correctness.

In the process of debugging this diff, I found that the most useful test
is not the absence of unexpected output in wrong places, but the
presence of all the expected output in the right places. In almost
all cases, when any code that used to write to the current output
stream was supposed to be updated to write to the specified stream,
but wasn't so updated, it is the absence of that output in the file
that the specified stream goes to that causes a problem, which is
generally reported as a parse failure by the code that consumes
that file.

There are two main reasons why such a report wouldn't happen:
(a) the missing output contains only white space, which
the parser ignores, and (b) the current output stream is the one
that we wanted to specify explicitly. The first can cause annoyances
but not showstopper problems. And, in places where the compiler
used to redirect output temporarily to a file, but output pred now
takes an explicit stream, I deleted the output redirect code
specifically to make any failure more visible.

(A third possible reason is that the file from which some parts are missing
is never read, but I don't think our testing framework allows that.)
 
>> 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.

I modified the comment to clarify that "specifying" includes implicitly
specifying the current stream (i.e. not passing a stream at all).

> That should be caught by --warn-implicit-stream-calls.

Yes, it should be, but it isn't :-(

> There would be an inconsistency with the io module predicates anyway.

Agreed.

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

Yes, but it would be easy enough to add a couple of fold predicates
specialized to pass a stream argument just before the last (or only)
accumulator pair.

> The inconsistency for the relatively few calls to add_list should be
> acceptable.

I think so too.

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

Thank you very much.

I followed all your other suggestions.

Zoltan.


More information about the reviews mailing list