[m-dev.] for review: rewrite of termination analysis (part 1)

[Fergus Henderson]

On 22-Dec-1997, Zoltan Somogyi <zs at cs.mu.oz.au> wrote:
> 
> Fergus, please review this. (I have already taken your very partial review
> into consideration,

You seem to have missed a few things from my partial review;

> % term_errors.m

> :- type termination_error
...
>       ;       no_eqns

Please document the meaning of `no_eqns'.

> term_errors__description(inf_call(CallerPPId, CalleePPId),
> 		Single, Module, Pieces, no) :-
> 	(
> 		Single = yes(PPId),
> 		require(unify(PPId, CallerPPId), "caller outside this SCC"),

I think you should spell out "SCC".

(Ditto in several other places.)

> term_errors__description(imported_pred, _, _, Pieces, no) :-
> 	Pieces = ["It", "contains", "one", "or", "more",
> 		"predicates", "and/or", "functions",
> 		"imported", "from", "another", "module."].

Should that be

 	Pieces = ["It", "contains", "one", "or", "more",
		"calls", "to", "predicates", "and/or", "functions",
	        ^^^^^^^^^^^^^^
 		"imported", "from", "another", "module."].
		
?

> term_errors__description(cycle(_StartPPId, CallSites), _, Module, Pieces, no) :-
> 	( CallSites = [DirectCall] ->
> 		term_errors__describe_one_call_site(DirectCall, Module, Site),
> 		Pieces = ["At", "the", "recursive", "call", "at", Site,
> 			"the", "arguments", "are", "not", "guaranteed",
> 			"to", "decrease", "in", "size."]
> 	;
> 		Pieces1 = ["in", "the", "recursive", "cycle",
> 			"through", "the", "calls", "at"],

s/"in"/"In"/

> :- pred term_errors__describe_one_pred_name(pred_id::in, module_info::in,
> 	string::out) is det.
> 
> term_errors__describe_one_pred_name(PredId, Module, Piece) :-
> 	module_info_pred_info(Module, PredId, PredInfo),
> 	pred_info_module(PredInfo, ModuleName),
> 	pred_info_name(PredInfo, PredName),
> 	pred_info_arity(PredInfo, Arity),
> 	pred_info_get_is_pred_or_func(PredInfo, PredOrFunc),
> 	(
> 		PredOrFunc = predicate,
> 		PredOrFuncPart = "predicate ",
> 		OrigArity = Arity
> 	;
> 		PredOrFunc = function,
> 		PredOrFuncPart = "function ",
> 		OrigArity is Arity - 1
> 	),
> 	string__int_to_string(OrigArity, ArityPart),
> 	string__append_list([
> 		PredOrFuncPart,
> 		ModuleName,
> 		":",
> 		PredName,
> 		"/",
> 		ArityPart
> 		], Piece).

As I noted previously, this duplicates code in hlds_out__write_pred_id.
I think you should at least include a comment in both places saying
that if you change one part then you will need to change the other.

> :- pred term_errors__describe_one_call_site(pair(pred_proc_id,
> 	term__context)::in, module_info::in, string::out) is det.
> 
> term_errors__describe_one_call_site(PPId - Context, Module, Piece) :-
> 	term_errors__describe_one_proc_name(PPId, Module, ProcName),
> 	Context = term__context(FileName, LineNumber),
> 	string__int_to_string(LineNumber, LineNumberPart),
> 	string__append_list([
> 		ProcName,
> 		" at ",
> 		FileName,
> 		":",
> 		LineNumberPart
> 		], Piece).

Term contexts should always be printed in the left-hand margin,
so that tools like `emacs' and `error' can parse them.

I think you should at very least add an XXX comment here.

> :- type pass1_result
> 	--->	ok(
> 			list(path_info),
> 					% One entry for each path through the
> 					% code.
> 			used_args,
> 					% The next output_supplier map.
> 			list(term_errors__error)
> 					% There is any entry in this list for
> 					% each procedure in the SCC in which
> 					% the sey of active vars were not
> 					% a subset of the input arguments.

s/any/an/
s/sey/set/
s/were/is/ (or perhaps s/were/was/)

> 	% User_type could be a type_info, which should be called size 0.
> 	% This is not a big problem, as most type_infos are input.  
> 
> :- pred zero_size_type_category(builtin_type, type, module_info, bool).
> :- mode zero_size_type_category(in, in, in, out) is det.
> 
> zero_size_type_category(int_type, _, _, yes).
> zero_size_type_category(char_type, _, _, yes).
> zero_size_type_category(str_type, _, _, yes).
> zero_size_type_category(float_type, _, _, yes).
> zero_size_type_category(pred_type, _, _, no).
> zero_size_type_category(enum_type, _, _, yes).
> zero_size_type_category(polymorphic_type, _, _, no).
> zero_size_type_category(user_type, _, _, no).

The comment above is wrong, I think: type_info should not be called size 0.

Apart from the above, that looks fine.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>   |  "I have always known that the pursuit
WWW: <http://www.cs.mu.oz.au/~fjh>   |  of excellence is a lethal habit"
PGP: finger fjh at 128.250.37.3         |     -- the last words of T. S. Garp.