for review: changes to termination analysis and stack tracing [1/3]

[Fergus Henderson]

On 02-Dec-1997, Zoltan Somogyi <zs at cs.mu.oz.au> wrote:
> 
> % term_errors.m

> :- type termination_error
...
> 	;	no_eqns

Please document the meaning of `no_eqns'.

> % An error is considered a simple error if it is likely that the error is
> % caused by the analysis failing, instead of being due to a programming
> % error. XXX this is not how it is used
> :- pred simple_error(term_errors__termination_error).
> :- mode simple_error(in) is semidet.
...
> simple_error(horder_call).
> simple_error(pragma_c_code).
> simple_error(imported_pred).
> simple_error(can_loop_proc_called(_, _)).
> simple_error(horder_args(_, _)).
> simple_error(does_not_term_pragma(_)).

Hmm, why should `does_not_term_pragma' and `can_loop_proc_called'
be considered simple errors?

> term_errors__output_same_SCC(PredId, Module, Context, ForHLDSDump) -->
> 	io__write_string("it is in the same SCC as the\n"),
> 	maybe_write_string(ForHLDSDump, "% "),
> 	prog_out__write_context(Context),
> 	io__write_string("  "),
> 	hlds_out__write_pred_id(Module, PredId).

If that message is supposed to be something that users will see,
I think you should spell out "SCC".

> term_errors__report_term_errors(SCC, Errors, Module) -->
> 	{ get_context_from_scc(SCC, Module, Context) },
> 	( { SCC = [PPId] } ->
> 		{ Pieces0 = ["Termination", "of"] },
> 		{ term_errors__describe_one_proc_name(PPId, Module, PredName) },
> 		{ list__append(Pieces0, [PredName], Pieces1) },

It would be simpler to just write

 		{ term_errors__describe_one_proc_name(PPId, Module, PredName) },
 		{ Pieces1 = ["Termination", "of", PredName] },

instead of calling list__append.

> 		{ Pieces2 = ["set", "to", "infinity", "the",
> 			"following", "reason:"] },
...
> 		{ Pieces2 = ["set", "to", "infinity", "the",
> 			"following", "reasons:"] },

Is there a "for" missing?
("Set to infinity _for_ the following reason"?)

> term_errors__description(pragma_c_code, _, _, Pieces, no) :-
> 	Pieces = ["It", "contains", "a", "pragma", "c_code()", "declaration."].

Please change that to
	Pieces = ["It", "contains", "a", "`pragma c_code'", "declaration."].

> term_errors__description(imported_pred, _, _, Pieces, no) :-
> 	Pieces = ["it", "contains", "imported", "code."].

Is "It contains imported code" correct?  Would it be better
to say "It contains a call to an imported procedure"?
Also, I suggest "code imported from another module" instead of
"imported code".

> term_errors__description(horder_args(CallerPPId, CalleePPId), Single, Module,
> 		Pieces, no) :-
> 	(
> 		Single = yes(PPId),
> 		require(unify(PPId, CallerPPId), "caller outside this SCC"),
> 		Piece1 = "It"
> 	;
> 		Single = no,
> 		term_errors__describe_one_proc_name(CallerPPId, Module, Piece1)
> 	),
> 	Piece2 = "calls",
> 	term_errors__describe_one_proc_name(CalleePPId, Module, CalleePiece),
> 	Pieces3 = ["with", "some", "higher", "order", "arguments."],
> 	Pieces = [Piece1, Piece2, CalleePiece | Pieces3].

How about `"one", "or", "more"' instead of `"some"'?

> 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"/

> term_errors__description(no_eqns, _, _, Pieces, no) :-
> 	Pieces = ["The", "analysis", "was", "unable", "to", "form", "any",
> 		"constraints", "between", "the", "arguments", "of", "the",
> 		"procedures", "in", "this", "SCC."].

I think you should spell out SCC.

> :- 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).
>
> :- pred term_errors__describe_one_proc_name(pred_proc_id::in, module_info::in,
> 	string::out) is det.
> 
> term_errors__describe_one_proc_name(proc(PredId, ProcId), 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),
> 	proc_id_to_int(ProcId, ProcIdInt),
> 	string__int_to_string(ProcIdInt, ProcIdPart),
> 	string__append_list([
> 		PredOrFuncPart,
> 		ModuleName,
> 		":",
> 		PredName,
> 		"/",
> 		ArityPart,
> 		" mode ",
> 		ProcIdPart
> 		], Piece).

I detect some code duplication here.

`describe_one_proc_name' could be implemented by calling
`describe_one_pred_name'.

As well as the duplication between the above two predicates,
they also duplicate code in hlds_out__write_pre_id. If you can't 
figure out how to avoid the duplication, then 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.

Peter Schachte has in fact recently committed a change to
hlds_out__write_pred_id which is not reflected in your code
above, but which should be.  He changed it to print out
reasonable names for special predicates (unification, compare/3,
index), e.g. for errors in user-defined equality predicates.

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


... to be continued.

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