[m-rev.] for review: dependency tracking

[Mark Brown]

On 19-Apr-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> RCS file: /home/mercury1/repository/mercury/browser/declarative_debugger.m,v
> retrieving revision 1.23
> diff -u -b -r1.23 declarative_debugger.m
> --- browser/declarative_debugger.m	4 Apr 2002 06:00:06 -0000	1.23
> +++ browser/declarative_debugger.m	17 Apr 2002 06:06:07 -0000

> +:- type dependency_chain_start(R)
> +	--->	chain_start(
> +			start_loc(R),
> +			int,		% The argument number of the selected
> +					% position in the full list of
> +					% arguments, including the
> +					% compiler-generated ones.
> +			R,		% The id of the exit goal,
> +					% if start_loc is cur_goal
> +					% and the id of the call goal
> +					% if start_loc is parent_goal.

That should be exit node and call node, rather than exit goal and call goal.
Also, the value that is stored here is the id of the node _preceding_ each
of those two.

> +			maybe(goal_path),
> +					% No if start_loc is cur_goal;
> +					% and yes wrapped around the goal path
> +					% of the call in the parent procedure
> +					% if start_loc is parent_goal.
> +			maybe(proc_rep)	% The body of the procedure indicated
> +					% by start_loc.
> +		).
> +
> +:- type start_loc(R)
> +	--->	cur_goal
> +	;	parent_goal(R, trace_node(R)).
> +
> +:- type goal_and_path	--->	goal_and_path(goal_rep, goal_path).
> +
> +:- type goal_and_path_list ==	list(goal_and_path).
> +
> +:- type annotated_primitive(R)
> +	--->	primitive(
> +			string,		% filename
> +			int,		% line number
> +			list(var_rep),	% vars bound by the atomic goal
> +			atomic_goal_rep,% the atomic goal itself
> +			goal_path,	% its goal path
> +			maybe(edt_node(R))
> +					% if the atomic goal is a call,
> +					% the id of the call's exit event
> +		).

A better choice for the type of the last field would be maybe(R).  Likewise,
the contour produced by materialize_contour should have the type
assoc_list(R, trace_node(R)).  The reason for this is that in the term
dynamic(Ref), which is of type edt_node(R), the Ref is meant to be
either an exit, fail or exception event.  But here we want to make use
of call nodes, so if we leave the types as they are we wind up constructing
terms of type edt_node(R) which are not really edt nodes at all.

When I add the other documentation you asked for earlier, I'll also document
this point about edt_node.

> +
> +:- pred find_chain_start(wrap(S)::in, edt_node(R)::in,
> +	arg_pos::in, term_path::in, dependency_chain_start(R)::out)
> +	is det <= annotated_trace(S, R).

The types of the first two arguments could just be S and R, respectively,
which would simplify the code a bit.  The same applies to various other
predicates called from trace_dependency.

> +
> +find_chain_start(wrap(Store), dynamic(Ref), ArgPos, TermPath,
> +		ChainStart) :-
> +	det_edt_return_node_from_id(Store, Ref, Node),
> +	(
> +		Node = exit(ExitPrec, CallId, _, ExitAtom, _),
> +		call_node_from_id(Store, CallId, Call),
> +		Call = call(CallPrec, _, CallAtom, _, _, _, ProcRep,
> +			CallPathStr),
> +		( trace_atom_subterm_is_ground(CallAtom, ArgPos, TermPath) ->
> +			path_from_string_det(CallPathStr, CallPath),
> +			StartLoc = parent_goal(CallId, Call),
> +			absolute_arg_num(ArgPos, CallAtom, ArgNum),
> +			StartId = CallPrec,
> +			StartPath = yes(CallPath),
> +			parent_proc_rep(wrap(Store), dynamic(CallId), StartRep)
> +		; trace_atom_subterm_is_ground(ExitAtom, ArgPos, TermPath) ->
> +			StartLoc = cur_goal,
> +			absolute_arg_num(ArgPos, ExitAtom, ArgNum),
> +			StartId = ExitPrec,
> +			StartPath = no,
> +			StartRep = ProcRep

It would be clearer and more concise to put the above 'then' branches
(which are also duplicated beneath here) into separate functions.  The
names of the functions could be, say, find_chain_start_inside and
find_chain_start_outside.

> +
> +:- pred step_left_to_call(S::in, R::in, trace_node(R)::in, R::out) is det
> +	<= annotated_trace(S, R).

Since the only call to this is followed by a call to call_node_from_id,
it would probably be better to make the type of the output argument
trace_node(R) rather than R.  This way, the second argument would no longer
be required.

> +
> +step_left_to_call(Store, Id, Node, ParentCallId) :-
> +	( Node = call(_, _, _, _, _, _, _, _) ->
> +		ParentCallId = Id
>  		;
> -			Mode = subterm_out,
> -			MaybeCallArgs = no
> -		->
> -			%
> -			% Headvars have the same number as their argument
> -			% position.
> -			%
> -			VarRep = ArgPos
> +		( Node = neg(NegPrec, _, _) ->
> +			PrevNodeId = NegPrec
>  		;
> -			error("trace_dependency: contour mismatch")
> +			PrevNodeId = step_left_in_contour(Store, Node)

We could modify step_left_in_contour to handle the neg case, so that it
doesn't have to be specially handled here (and below).  The code for
step_left_in_contour assumes that a neg node will only be reached if an
exception is thrown inside a negation, but now that we use that function
here the assumption is false.  I can't think of any problems that removing
this assumption might cause.  What do you think?

>  		),
> -		Origin = find_subterm_origin(AtomInfo, VarRep, TermPath)
> +		det_trace_node_from_id(Store, PrevNodeId, PrevNode),
> +		step_left_to_call(Store, PrevNodeId, PrevNode, ParentCallId)
>  	).
>  

This completes this round of reviewing.

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