[m-rev.] for review: specialize all calls to {string, io}.format

[Zoltan Somogyi]

On Sun, 30 Nov 2014 17:31:37 +1100 (AEDT), Julien Fischer <jfischer at opturion.com> wrote:
> > Should we implicitly import string.format.m and string.parse_util.m
> > (the two modules needed by the specialization of calls to string.format
> > and io.format) in all modules? In all modules that import string.m?
> > Or just the modules that may call a format predicate, as in this diff?
> 
> I think what you have in this diff is fine.  The only issue, as I understand
> it, is that you may unnecessarily implicitly import the above modules into a
> module that defines its own predicates or functions named 'format'.  That seems
> as though it would be (1) fairly rare and (2) harmless.

I did it the way I did because it was the closest "cultural" fit to the
other code that discovers the need for implicit imports. Actually, there
is an argument against it. The cost of scanning for the need for implicit
imports used to be checking a small and bounded number of things
per item, but we now have to traverse every part of every clause, which
(for large clauses) can be an arbitrarily higher amount of work.

I would actually prefer the second solution, but post-processing the
import list is tricky, since the code that handles imports is currently
a mess.

> I've had a look through the code that I have that uses streams and could not
> find anything that would be useful as a test.  Even if such code existed I
> don't think it would tell you anything useful anyway.  Code using streams is
> almost always intended to be generic in the underlying stream and in the
> absence of information about the relative costs of appending strings versus
> method calls to 'put' for the _specific_ instance stream in question you cannot
> decide between the two alternatives above.

Do you have code that I can use as a more substantial test of correctness,
even if it doesn't help decide which implementation approach to pick?

> On balance, I think that you should handle calls to string_writer.format as per
> (1) above since that approach is not going to be any worse than what we already
> do at runtime.  Short of doing lots of type class method specialisation and
> inlining or in the absence of, for example, profiling feedback you won't be
> able to do better.

Ok, I will do that, once you (or someone else) gives me a substantial
test case.

> > Or should we add a version of maybe_error in which the error alternative
> > lists one or more errors?
> 
> I have no objection to adding such a type.

I will add it, then.

> > As for whether the compiler should , by default,print more than one
> > error message for one call to a format predicate, I think we will need
> > to see the second (and maybe later) messages from real, non-test errors
> > to see whether they are useful or misleading.
> 
> Agreed.

I turned off printing the second and later messages for the commit,
because I found that for the test cases, they were misleading. I think
we will need to work on recovering from each error as much as possible
before we experiment with turning it on. (At the moment, we just give up
as soon as we find an error and don't even try to clean up, so it is
not surprising that later error messages are misleading.)

> In C grades retrieving the current output stream involves a function call plus
> an access to a thread local mutable (in .par grades).  (I didn't look at what
> the non-C grades do but it's presumably something similar).  If optimizing the
> retrieval of the implicit stream were worth it then it would be worth doing for
> for all conjunctions of "primitive" I/O operations that cannot change the
> current output stream, not just the ones introduced by this module.  In short,
> it's a separate issue from what this change does.

OK, I added a comment to that effect.

> > diff --git a/library/Mmakefile b/library/Mmakefile
> > index b7ad2a7..931ce73 100644
> > --- a/library/Mmakefile
> > +++ b/library/Mmakefile

> > +.PHONY: check_doc_undoc
> > +check_doc_undoc: MODULES_DOC MODULES_UNDOC *.m
> > +	@{																\
> > +		cat MODULES_DOC MODULES_UNDOC | sort > FILES_VIA_MODULES;	\
> > +		ls *.m > FILES_VIA_LS;										\
> > +		if diff -u FILES_VIA_MODULES FILES_VIA_LS; then				\
> > +			true;													\
> > +		else														\
> > +			false;													\
> > +		fi;															\
> > +		rm -f FILES_VIA_MODULES FILES_VIA_LS;						\
> > +	}
> > +
> 
> The spacing before the line continuation characters looks awry there.

Mixing ts=4 and ts=8; fixed, and an explicit ts=8 added.

I also followed your other suggestions.

Zoltan.