[m-dev.] For review: handling one by one variable retrieval within the external debugger

[Fergus Henderson]

Thanks Erwan.

I have a few comments.  Could you please send a relative diff
when you have addressed the comments below?

(That is, could you please make a backup of your current sources
before you change anything relating to the comments below,
and then when you've finished, use `diff -u' to get a diff
between the backup and your final sources.)

On 28-Jul-1998, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> Index: std_util.m
> ===================================================================
> RCS file: /home/mercury1/repository/mercury/library/std_util.m,v
> retrieving revision 1.124
> diff -u -r1.124 std_util.m
> --- std_util.m	1998/07/22 07:42:06	1.124
> +++ std_util.m	1998/07/28 07:35:18
> @@ -1319,6 +1319,8 @@
>  }
>  ").
>  
> +:- pragma export(type_name(in) = out, "ML_type_name").

Can you add a comment here please?

runtime/*:
> +/*
> +** This function returns the list of types of currently live variables.
> +*/
> +
> +static Word
> +MR_trace_make_type_list(const MR_Stack_Layout_Label *layout)
> +{
> +	int 				var_count;
> +	const MR_Stack_Layout_Vars 	*vars;
> +	int				i;
> +	const char			*name;
> +	MR_Stack_Layout_Var*		var;
> +	Word				type_info;
> +	String		      		type_info_string;
> +
> +	Word				type_list;
> +
> +	var_count = layout->MR_sll_var_count;
> +	vars = &layout->MR_sll_var_info;
> +
> +	restore_transient_registers();
> +	type_list = list_empty();
> +	save_transient_registers();
> +	for (i = var_count - 1; i >= 0; i--) {
> +
> +		name = MR_name_if_present(vars, i);
> +		var = &vars->MR_slvs_pairs[i];
> +
> +		if ((strncmp(name, "TypeInfo", 8) == 0)
> +		|| (strncmp(name, "ModuleInfo", 10) == 0)
> +		|| (strncmp(name, "HLDS", 4) == 0)
> +		|| !MR_trace_get_type(var, NULL, &type_info))
> +		{
> +			continue;
> +		}

I'm sure I've seen that code before.  You should abstract it out
into a function rather than just doing cut-and-paste reuse.

> +		restore_transient_registers();
> +		type_info_string = MR_type_name(type_info);
> +		type_list = list_cons(type_info_string, type_list);
> +		save_transient_registers();
> +	}
> +
> +	return type_list;
> +}
> +
> +
> +/*
> +** This function returns the requested live variable.
> +*/
> +
> +static Word
> +MR_trace_make_nth_var(const MR_Stack_Layout_Label *layout, Word
> debugger_request)

That line didn't fit in 79 columns.

> +	var_number = MR_get_var_number(debugger_request);
> +		/* debugger_request should be of the form:
> current_nth_var(var_number) */

Likewise.

> +	vars = &layout->MR_sll_var_info;
> +	name = MR_name_if_present(vars, var_number);
> +	var = &vars->MR_slvs_pairs[var_number];
> +
> +	restore_transient_registers();
> +	incr_hp(univ, 2);
> +
> +	if ((strncmp(name, "TypeInfo", 8) == 0)
> +	    || (strncmp(name, "ModuleInfo", 10) == 0)
> +	    || (strncmp(name, "HLDS", 4) == 0)
> +	    || !MR_trace_get_type_and_value(var, NULL, &type_info, &value))

Again, that code should not be duplicated.

> +	  {
> +	    field(mktag(0), univ, UNIV_OFFSET_FOR_TYPEINFO) = type_info;
> +	    field(mktag(0), univ, UNIV_OFFSET_FOR_DATA) = value;
> +	  } else {
> +	    field(mktag(0), univ, UNIV_OFFSET_FOR_TYPEINFO) = type_info;
> +	    field(mktag(0), univ, UNIV_OFFSET_FOR_DATA) = value;
> +	  }

That code looks wrong -- why are both the `then' and the `else' parts the
same?

> +/*
> +** This function is called only when debugger_request = current_nth_var(n).
> +** It returns the integer 'n'.
> +*/
> +
> +/* XXX note for the reviewer: is there a way to do that in C ? */
> +
> +static int
> +MR_get_var_number(Word debugger_request)
> +{
> +	int result;
> +
> +	result = MR_DI_get_var_number(debugger_request);
> +	return result;
> +}

I'd just write that as

	static int
	MR_get_var_number(Word debugger_request)
	{
		return MR_DI_get_var_number(debugger_request);
	}

Apart from those points, it looks OK.

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