[m-dev.] for review: new method of handling failures, part 1 of 6

[Zoltan Somogyi]

> Firstly, this version is a major improvement!
> The code is much more modular and much better encapsulated.

Thanks.

> Some of these comments below may relate to existing code that was not
> modified in this change.

Actually, most of them do :-(

> > 	% Put the given resume point into effect, by pushing it on to
> > 	% the resume point stack, and if necessary overriding the redoip
> > 	% of the top frame.
> > 
> >:- pred code_info__effect_resume_point(resume_point_info::in, code_model::in,
> > 	code_tree::out, code_info::in, code_info::out) is det.
> 
> The comment would be clearer, IMHO, if it started with "Generate code to ".

It shouldn't, since only the overriding part involves the generation of code.

> > code_info__enter_simple_neg(ResumeVars, GoalInfo, FailInfo0) -->
> > 	code_info__get_fail_info(FailInfo0),
> > 		% The only reason why we push a resume point at all
> > 		% is to protect the variables in ResumeVars from becoming
> > 		% unknown; by including them in the domain of the resume point
       ,
> > 		% we guarantee that the will become zombies instead of unknown
> > 		% if they die in the pre- or post-goal updates.
> > 		% Therefore the only part of ResumePoint that matters
> > 		% is the set of variables in the resume map; the other
> > 		% parts of ResumePoint (the locations, the code address)
> > 		% will not be referenced.
> 
> What's a zombie?  What's the difference between a zombie and unknown?

That is covered in allocation.html, which I have put in a pointer to.

> > :- pred code_info__match_resume_loc(resume_map::in, resume_map::in) is sem
       idet.
> > 
> > code_info__match_resume_loc(Map, Locations0) :-
> > 	map__keys(Map, KeyList),
> > 	set__list_to_set(KeyList, Keys),
> > 	map__select(Locations0, Keys, Locations),
> > 	map__to_assoc_list(Locations, List),
> > 	\+ (
> > 		list__member(Thingy, List),
> > 		\+ (
> > 			Thingy = Var - Actual,
> > 			map__search(Map, Var, Rvals),
> > 			set__subset(Rvals, Actual)
> > 		)
> > 	).
> 
> `Thingy' is a rather non-descriptive variable name.
> I suggest unfolding the unification `Thingy = Var - Actual' in
> the call to `list__member'.

Done.

> I also suggest using `all' and `=>' rather than nested '\+'.
> 
> 	all [Var, Actual] (
> 		list__member(Var - Actual, List) => (
> 			map__search(Map, Var, Rvals),
> 			set__subset(Rvals, Actual)
> 		)
> 	)

We haven't actually driven a stake through the heart of SICStus debugging yet,
and I am not absolutely sure that this would be OK in SICStus, so I will leave
it as it is for the time being.

Zoltan.