[m-rev.] Fixes to pprint and change for --dump-mlds

[Mark Brown]

This looks good.  I haven't finished the review yet, but here is a first
round of comments.

On 23-Oct-2001, Ralph Becket <rafe at cs.mu.OZ.AU> wrote:
> Estimated hours taken: 2.5
> Branches: main
> 
> Fixed a performance bug in pprint and a couple of formatting bugs.

There should be regression tests for the formatting bug fixes.  Does
this change affect any of the existing tests?  If so, the expected
output of the test may need to be updated.

(I'm not sure how familiar you are with our setup, so here's a rough
outline of what steps to take:

Check out the tests module ('cvs co tests').  You can do this as a child
or sibling to the mercury directory, and our scripts will work.

Run a bootcheck, which also runs all of the tests and tells you in which
test directory failures occurred ('tools/bootcheck --help' for more info).

Some documentation of how individual tests are run can be found in
tests/Mmake.common, but also look in the Mmakefile in the subdirectories
for additional rules.  If a test foo fails because the output (in foo.out)
doesn't match the expected output (in one of foo.exp*), the diff is saved
(in foo.res).  If the output is actually correct, you should copy its
contents into the appropriate foo.exp* file, and include the diff of
this file in the diff you post for review.

To add a new test case, you generally just need to add the required
files to the appropriate tests directory and update the Mmakfile there.
Tests of features in the standard library would usually go in the
hard_coded tests directory.)

> Changed the --dump-mlds option in the compiler to use pprint
> rather than just dumping unformatted data.
> 
> library/pprint.m:
> 	Fixed a performance bug in be/5 which was not completely
> 	tail recursive.  This caused stack overflow when pretty
> 	printing large terms.
> 	Fixed the default formatting for lists in particular and
> 	terms in general where the indentation on line-wrapping
> 	could go wrong.
> compiler/mercury_compile.m:
> 	Changed the --dump-mlds option to use pprint rather than
> 	dump unformatted data.
> 
> 
> Index: pprint.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/pprint.m,v
> retrieving revision 1.5
> diff -u -r1.5 pprint.m
> --- pprint.m	26 Apr 2001 14:55:13 -0000	1.5
> +++ pprint.m	23 Oct 2001 08:22:00 -0000
> @@ -352,7 +352,8 @@
>  
>  best(W, K, X)           = Best
>  :-
> -    be(W, K, ["" - X], Best, []).
> +    be(W, K, ["" - X], [], RevBest),
> +    Best = list__reverse(RevBest).

Ah, of course!  Regardless of how it is specified, the list will be built
bottom up by the current Mercury implementation.  Given that the
elements of the simple_doc list are determined in a particular order,
page-reading order, these elements need to be added to the list in that
order if use of the det stack is to be avoided.  Which means that the list
specified by be/5 must be in *reverse* page-reading order.

This raises a (minor) issue though: is the reversed list still of type
simple_doc?  I don't think it should be, since it is intended to be
interpreted differently.  So you could define a rev_simple_doc type, or
just use list(string) in the type declarations of be/5 and push_string/3.

Also, the new list order should be documented, e.g. in the comment above
the code for be/5.

>  
>  %------------------------------------------------------------------------------%
>  
> @@ -365,15 +366,32 @@
>      % simple_doc type.
>  
>  :- pred be(int, int, list(pair(string, doc)), simple_doc, simple_doc).
> -:- mode be(in, in, in, out, in) is det.
> +:- mode be(in, in, in, in, out) is det.
> +
> +be(_, _, [])                      -->
> +    [].
> +
> +be(W, K, [_ - 'NIL'         | Z]) -->
> +    be(W, K, Z).
> +
> +be(W, K, [I - 'SEQ'(X, Y)   | Z]) -->
> +    be(W, K, [I - X, I - Y | Z]).
> +
> +be(W, K, [I - 'NEST'(J, X)  | Z]) -->
> +    be(W, K, [extend(I, J) - X | Z]).
> +
> +be(W, K, [I - 'LABEL'(L, X) | Z]) -->
> +    be(W, K, [string__append(I, L) - X | Z]).
> +
> +be(W, K, [_ - 'TEXT'(S)     | Z]) -->
> +    push_string(S),
> +    be(W, (K + string__length(S)), Z).
> +
> +be(W, _, [I - 'LINE'        | Z]) -->
> +    push_string("\n"),
> +    push_string(I),
> +    be(W, string__length(I), Z).
>  
> -be(_, _, [], Out, In)             :-  Out = list__reverse(In).
> -be(W, K, [_ - 'NIL'         | Z]) --> be(W, K, Z).
> -be(W, K, [I - 'SEQ'(X, Y)   | Z]) --> be(W, K, [I - X, I - Y | Z]).
> -be(W, K, [I - 'NEST'(J, X)  | Z]) --> be(W, K, [extend(I, J) - X | Z]).
> -be(W, K, [I - 'LABEL'(L, X) | Z]) --> be(W, K, [string__append(I, L) - X | Z]).
> -be(W, K, [_ - 'TEXT'(S)     | Z]) --> [S], be(W, (K + string__length(S)), Z).
> -be(W, _, [I - 'LINE'        | Z]) --> ["\n", I], be(W, string__length(I), Z).
>  be(W, K, [I - 'GROUP'(X)    | Z]) -->
>      ( if { flattening_works(X, Z, W - K) } then
>          be(W, K, [I - flatten(X) | Z])
> @@ -381,6 +399,13 @@
>          be(W, K, [I - X | Z])
>      ).
>  
> +
> +
> +:- pred push_string(string, simple_doc, simple_doc).
> +:- mode push_string(in, in, out) is det.
> +
> +push_string(S, Ss, [S | Ss]).
> +
>  %------------------------------------------------------------------------------%

I haven't reviewed the rest of the changes to this file.

> Index: mercury_compile.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/compiler/mercury_compile.m,v
> retrieving revision 1.220
> diff -u -r1.220 mercury_compile.m
> --- mercury_compile.m	2 Oct 2001 07:09:43 -0000	1.220
> +++ mercury_compile.m	23 Oct 2001 08:18:56 -0000
> @@ -33,6 +33,7 @@
>  :- import_module equiv_type, make_hlds, typecheck, purity, polymorphism, modes.
>  :- import_module switch_detection, cse_detection, det_analysis, unique_modes.
>  :- import_module stratify, simplify.
> +:- import_module pprint.
>  
>  	% high-level HLDS transformations
>  :- import_module check_typeclass, intermod, trans_opt, table_gen, (lambda).
> @@ -3957,10 +3958,8 @@
>  	maybe_flush_output(Verbose),
>  	io__tell(DumpFile, Res),
>  	( { Res = ok } ->
> -		% XXX the following doesn't work, due to performance bugs
> -		% in pprint:
> -		%	pprint__write(80, pprint__to_doc(MLDS)),
> -		io__print(MLDS), io__nl,
> +		pprint__write(80, pprint__to_doc(MLDS)),
> +		io__nl,
>  		io__told,
>  		maybe_write_string(Verbose, " done.\n"),
>  		maybe_report_stats(Stats)

This change is fine, provided you have tested that the performance is
acceptable.  Doing an MLDS dump of compiler/make_hlds.m might make a
good test case.

Please address the above issues and post another diff, including test
cases for where formatting used to go wrong.  I'll review the remaining
changes after that (since I'll be able to review the formatting changes
more effectively if I review the test cases at the same time).

Cheers,
Mark.

--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------