[m-rev.] for review: dependency tracking

[Mark Brown]

This is the next installment of review comments.  There is still some
parts which I haven't reviewed fully; I'll complete the review soon.

Cheers,
Mark.

On 19-Apr-2002, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> Index: browser/declarative_execution.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/browser/declarative_execution.m,v
> retrieving revision 1.16
> diff -u -b -r1.16 declarative_execution.m
> --- browser/declarative_execution.m	4 Apr 2002 06:00:06 -0000	1.16
> +++ browser/declarative_execution.m	16 Apr 2002 09:20:50 -0000
> @@ -29,95 +29,152 @@

...

>  	;	later_disj(
> -			R,			% Preceding event.
> -			goal_path_string,	% Path for this event.
> -			R			% Event of the first DISJ.
> +			last_disj_preceding	:: R,
> +						% Preceding event.
> +			last_disj_goal_path	:: goal_path_string,
> +						% Path for this event.
> +			last_disj_first		:: R
> +						% Event of the first DISJ.
>  		)

These field names should be "later_disj_*" rather than "last_disj_*".

> @@ -253,13 +310,34 @@
>  
>  %-----------------------------------------------------------------------------%
>  
> +:- type which_headvars
> +	--->	all_headvars
> +	;	only_user_headvars.
> +
> +:- pred maybe_filter_headvars(which_headvars::in, list(trace_atom_arg)::in,
> +	list(trace_atom_arg)::out) is det.
> +
> +:- func head_vars_presentation = which_headvars.

A better name for this function would be default_head_vars_presentation.

> +
> +:- pred is_user_vis_arg(trace_atom_arg::in) is semidet.
> +
> +:- pred select_arg_at_pos(arg_pos::in, list(trace_atom_arg)::in,
> +	trace_atom_arg::out) is det.
> +
> +:- pred absolute_arg_num(arg_pos::in, trace_atom::in, int::out)
> +	is det.

Arg, vars, pos and num are all abbreviations which are used fairly often
in our code, but vis is not.  I'd change the name of the predicate to
is_user_visible_arg.

> @@ -610,7 +688,7 @@
>  :- pragma export(trace_node_path(in, in) = out,
>  		"MR_DD_trace_node_path").
>  
> -trace_node_path(_, call(_, _, _, _, _, _, _)) = "".
> +trace_node_path(_, call(_, _, _, _, _, _, _, P)) = P.
>  trace_node_path(_, exit(_, _, _, _, _)) = "".
>  trace_node_path(_, redo(_, _)) = "".
>  trace_node_path(_, fail(_, _, _, _)) = "".

Now that it is possible to get to the (parent) goal path of interface
events, I think it would be worth doing this for all the interface events
here.  You don't need to as part of this change, since it wouldn't be used
anyway.  But it does make more sense than returning the empty string as
currently happens.

> @@ -865,21 +931,47 @@
>  	"Id = (MR_Word) NULL;"
>  ).
>  
> -
>  :- func construct_trace_atom(pred_or_func, string, int) = trace_atom.
>  :- pragma export(construct_trace_atom(in, in, in) = out,
>  		"MR_DD_construct_trace_atom").
>  
>  construct_trace_atom(PredOrFunc, Functor, Arity) = Atom :-
>  	Atom = atom(PredOrFunc, Functor, Args),
> -	list__duplicate(Arity, no, Args).
> +	list__duplicate(Arity, dummy_arg_info, Args).
> +
> +:- func add_trace_atom_arg_value(trace_atom, int, int, int, univ) = trace_atom.
> +:- pragma export(add_trace_atom_arg_value(in, in, in, in, in) = out,
> +		"MR_DD_add_trace_atom_arg_value").
> +
> +add_trace_atom_arg_value(atom(C, F, Args0), Num, ProgVisNum, ProgVis, Val)
> +		= atom(C, F, Args) :-
> +	Arg = arg_info(c_bool_to_merc_bool(ProgVis), ProgVisNum, yes(Val)),
> +	list__replace_nth_det(Args0, Num, Arg, Args).
> +
> +:- func add_trace_atom_arg_no_value(trace_atom, int, int, int) = trace_atom.
> +:- pragma export(add_trace_atom_arg_no_value(in, in, in, in) = out,
> +		"MR_DD_add_trace_atom_arg_no_value").
> +
> +add_trace_atom_arg_no_value(atom(C, F, Args0), Num, ProgVisNum, ProgVis)
> +		= atom(C, F, Args) :-
> +	Arg = arg_info(c_bool_to_merc_bool(ProgVis), ProgVisNum, no),
> +	list__replace_nth_det(Args0, Num, Arg, Args).

It would be worth documenting these two functions, particularly since they
take a Mercury integer that is meant to be interpreted as a C bool.

> Index: browser/declarative_user.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/browser/declarative_user.m,v
> retrieving revision 1.16
> diff -u -b -r1.16 declarative_user.m
> --- browser/declarative_user.m	4 Mar 2002 19:28:44 -0000	1.16
> +++ browser/declarative_user.m	16 Apr 2002 14:23:11 -0000
> @@ -95,15 +95,23 @@
>  		{ reverse_and_append(Skipped, [Node | Nodes], Questions) },
>  		query_user_2(Questions, [], Response, User1, User)
>  	;
> -		{ Command = browse(Arg) },
> -		browse_edt_node(Node, Arg, MaybeMark, User1, User2),
> +		{ Command = browse(ArgNum) },
> +		browse_edt_node(Node, ArgNum, MaybeMark, User1, User2),
>  		(
>  			{ MaybeMark = no },
>  			query_user_2([Node | Nodes], Skipped, Response, User2,
>  					User)
>  		;
>  			{ MaybeMark = yes(Mark) },
> -			{ Answer = suspicious_subterm(Node, Arg, Mark) },
> +			{ Which = head_vars_presentation },
> +			{
> +				Which = only_user_headvars,
> +				ArgPos = user_head_var(ArgNum)
> +			;
> +				Which = all_headvars,
> +				ArgPos = any_head_var(ArgNum)
> +			},
> +			{ Answer = suspicious_subterm(Node, ArgPos, Mark) },
>  			{ Response = user_answer(Answer) },
>  			{ User = User2 }
>  		)

A suggestion on naming: perhaps you could use UserArgPos and RealArgPos
to distinguish the two ways of numbering arguments.

> Index: tests/debugger/declarative/dependency.m
> ===================================================================
> RCS file: tests/debugger/declarative/dependency.m
> diff -N tests/debugger/declarative/dependency.m
> --- /dev/null	1 Jan 1970 00:00:00 -0000
> +++ tests/debugger/declarative/dependency.m	16 Apr 2002 08:13:31 -0000
> @@ -0,0 +1,76 @@

...

> +:- pred turn_on_origin_debug is det.
> +
> +:- pragma foreign_proc("C",
> +	turn_on_origin_debug,
> +	[will_not_call_mercury, promise_pure],
> +"
> +	extern	int	MR_DD_debug_origin;
> +
> +	MR_DD_debug_origin = 1;
> +").

This promise is a lie.  You should thread the io__state through here,
or else leave it impure and promise the caller to be pure.

> Index: trace/mercury_trace_vars.c
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_vars.c,v
> retrieving revision 1.41
> diff -u -b -r1.41 mercury_trace_vars.c
> --- trace/mercury_trace_vars.c	3 Apr 2002 07:08:22 -0000	1.41
> +++ trace/mercury_trace_vars.c	16 Apr 2002 14:57:23 -0000
> @@ -1152,6 +1175,31 @@
>  
>  	(*browser)((MR_Word) typeinfo, *value, caller, format);
>  	return NULL;
> +}
> +
> +MR_ConstString
> +MR_hlds_var_name(const MR_Proc_Layout *entry, int hlds_var_num)
> +{
> +	const char	*string_table;
> +	MR_Integer	string_table_size;
> +	int		offset;
> +
> +	string_table = entry->MR_sle_module_layout->MR_ml_string_table;
> +	string_table_size =
> +		entry->MR_sle_module_layout->MR_ml_string_table_size;
> +
> +	if (hlds_var_num > entry->MR_sle_max_named_var_num) {
> +		/* this value is a compiler-generated variable */
> +		return NULL;
> +	}
> +
> +		/* variable number 1 is stored at offset 0 */

That should read: "The offset of variable number 1 is stored at index 0",
or words to that effect.

> +	offset = entry->MR_sle_used_var_names[hlds_var_num - 1];
> +	if (offset > string_table_size) {
> +		MR_fatal_error("array bounds error on string table");
> +	}
> +
> +	return string_table + offset;
>  }
>  
>  MR_Completer_List *
> Index: trace/mercury_trace_vars.h
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/trace/mercury_trace_vars.h,v
> retrieving revision 1.19
> diff -u -b -r1.19 mercury_trace_vars.h
> --- trace/mercury_trace_vars.h	11 Mar 2002 19:45:48 -0000	1.19
> +++ trace/mercury_trace_vars.h	16 Apr 2002 14:56:49 -0000
> @@ -107,6 +109,17 @@
>  extern	const char	*MR_trace_list_vars(FILE *out);
>  
>  /*
> +** Return as a side effect the type and value of the value with the

The second occurrence of "value" should be "variable".

> +** specified HLDS number, in the specified locations, all of which must be
> +** non-NULL. If the variable isn't live or isn't known, return a non-null
> +** string giving the problem.
> +*/
> +
> +extern const char *	MR_trace_return_hlds_var_info(int hlds_num,
> +				MR_TypeInfo *type_info_ptr,
> +				MR_Word *value_ptr);
> +
> +/*
>  ** Return as a side effect the name, type and value of the specified
>  ** variable in the specified locations, except those which are NULL.
>  ** Variable number n must be in the range 1..MR_trace_var_count().
--------------------------------------------------------------------------
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
--------------------------------------------------------------------------