[m-dev.] for review: type specialization [2]

[Fergus Henderson]

On 17-Feb-1999, Simon Taylor <stayl at cs.mu.OZ.AU> wrote:
> 
> --- mercury_to_mercury.m	1998/12/06 23:43:48	1.152
> +++ mercury_to_mercury.m	1999/02/10 05:01:11
> @@ -32,6 +32,9 @@
>  				io__state, io__state).
>  :- mode convert_to_mercury(in, in, in, di, uo) is det.
>  
> +:- pred mercury_output_item(item, prog_context, io__state, io__state).
> +:- mode mercury_output_item(in, in, di, uo) is det.

Why is that exported?  The log message didn't mention that change.

> --- prog_io.m	1999/02/08 20:52:38	1.180
> +++ prog_io.m	1999/02/10 05:48:13
> @@ -110,21 +110,15 @@
>  :- pred search_for_file(list(dir_name), file_name, bool, io__state, io__state).
>  :- mode search_for_file(in, in, out, di, uo) is det.
>  
> -	% parse_item(ModuleName, VarSet, Term, MaybeItem)
> -	%
> -	% parse Term. If successful, MaybeItem is bound to the parsed item,
> -	% otherwise it is bound to an appropriate error message.
> -	% Qualify appropriate parts of the item, with ModuleName as the
> -	% module name.
> -:- pred parse_item(module_name, varset, term, maybe_item_and_context). 
> -:- mode parse_item(in, in, in, out) is det.
> -

Why is that no longer exported?  The log message didn't mention that
change.

>  	% parse_decl(ModuleName, VarSet, Term, Result)
>  	%
>  	% parse Term as a declaration. If successful, Result is bound to the
>  	% parsed item, otherwise it is bound to an appropriate error message.
>  	% Qualify appropriate parts of the item, with ModuleName as the module
>  	% name.
> +	% The item should not be a `:- pragma type_spec(...)'
> +	% declaration, since that would require a counter to be
> +	% threaded through.
>  :- pred parse_decl(module_name, varset, term, maybe_item_and_context).
>  :- mode parse_decl(in, in, in, out) is det.

You should explain that in more detail.

That restriction is quite undesirable and it would be better
if there was a way of making this change without introducing
that restriction.  The whole thing with counters is a bit ugly, really.
Isn't it reasonably straightforward to create a unique name
even for a specialization without needing a counter?

> +	% We use the empty module name ('') as the "root" module name;
> +	% when adding default module qualifiers in
> +	% parse_implicitly_qualified_{term,symbol}, if the default module
> +	% is the root module then we don't add any qualifier.
> +:- pred root_module_name(module_name::out) is det.

Why is that exported?  The log message doesn't mention that change.

> -:- pred read_items_loop(module_name, file_name,
> +:- pred read_items_loop(module_name, file_name, int,
>  			message_list, item_list, module_error, 
>  			message_list, item_list, module_error, 
>  			io__state, io__state).

You should document the meaning of the new `int' parameter.

One simple way to do that would be to give it type `type_spec_counter',
with

	% put comment here
	:- type type_spec_counter == int.

defined somewhere.

> -:- pred read_item(module_name, file_name, maybe_item_or_eof,
> +:- pred read_item(module_name, file_name, maybe_item_or_eof, int, int,
>  			io__state, io__state).

Likewise the ints here should be documented.

> +:- pred parse_item(module_name, varset, term,
> +		maybe_item_and_context, int, int). 
> +:- mode parse_item(in, in, in, out, in, out) is det.

Same again here.

> +:- pred parse_decl(module_name, varset, term, maybe_item_and_context,
> +		int, int).
> +:- mode parse_decl(in, in, in, out, in, out) is det.

And here.

> Index: compiler/prog_io_pragma.m
>  	% parse the pragma declaration. 
> -:- pred parse_pragma(module_name, varset, list(term), maybe1(item)).
> +:- pred parse_pragma(module_name, varset, list(term), maybe1(item), int, int).

And here.

> +parse_pragma_type(ModuleName, "type_spec", PragmaTerms, ErrorTerm, 
> +		VarSet0, Result, Counter0, Counter) :-
...
> +			TypeSubnResult = ok(TypeSubn),
> +			( MaybeName = yes(SpecializedName0) ->
> +				Counter = Counter0,
> +				SpecializedName = SpecializedName0
> +		    	;
...
> +				( MaybePredOrFunc = yes(PredOrFunc0) ->
> +					PredOrFunc = PredOrFunc0
> +				;
> +					% XXX This is just a guess.
> +					% The problem with this would
> +					% be a misleading entry in the
> +					% call profile, but there is a
> +					% context attached to the name,
> +					% so it isn't too much of a problem.
> +					PredOrFunc = predicate
> +				),
> +				make_pred_name_with_context(ModuleName,
> +					"TypeSpecOf", PredOrFunc,
> +					UnqualName, Line, Counter0,
> +					SpecializedName),
> +				Counter = Counter0 + 1

Ah, I see, so this is why you threaded the counter everywhere.

The "XXX" here already suggests that using make_pred_name_with_context
is not quite the right thing to do.  And the need to thread counters
through everything is another reason to avoid doing this.
I think it might be a better idea to implement something similar
to make_pred_name_with_context that takes a discriminated union
of `counter(int) ; type_subn(type_subst)' rather than just a counter.

> +		    	),
> +			varset__coerce(VarSet0, VarSet),

A comment here explaining why this is necessary/justified
would be helpful.

> +		   	Result = ok(pragma(type_spec(PredName,
> +				SpecializedName, Arity, MaybePredOrFunc,
> +				MaybeModes, TypeSubn, VarSet)))
> +		    ;
> +			TypeSubnResult = error(_, _),	
> +			Counter = Counter0,
> +			Result = error(
> +	"expected type substitution in `pragma type_spec(...)' declaration",
> +				TypeSubnTerm)
> +		)
> +	    ;
> +		    ArityOrModesResult = error(Msg, Term),
> +		    Result = error(Msg, Term),
> +		    Counter = Counter0
> +	    )
> +	;
> +	    Counter = Counter0,
> +	    Result = error(
> +		"wrong number of arguments in `pragma type_spec' declaration", 
> +		ErrorTerm)
> +	).

Here you're inconsistent about using `(...)'. 
Is there any reason for that?

> --- prog_io_util.m	1998/11/20 04:09:02	1.12
> +++ prog_io_util.m	1999/02/10 05:01:15

This file was not mentioned in the log message.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.