[m-dev.] For review: another change to the external debugger

[Fergus Henderson]

On 24-Jul-1998, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> There are 2 parts in that change

s/that/this/

> (1) make the retrieval of arguments and
> other slots done separately and (2) do not test whether or not an
> attribute is wanted to retrieve it. The rational for (2) is that for non
> argument attributes, as they are very small, we can always send them all
> to the socket without too much pain whereas testing each time whether or
> not an attribute is wanted can be very expensive (in time). Since the
> retrieval of arguments can be much more expensive (in place), we
> retrieve that slot separately (and we test in the debugger process if
> arguments are wanted).

Isn't that rationale the rationale for both (1) and (2), not just the
rationale for (2)?  If it's not, then what's the rationale for (1)?

I'm not sure I believe this rationale.  What makes you think that
testing whether or not an attribute is wanted is going to be more
expensive than sending it across a socket?

It may well be that your change does improve efficiency.
But if so, the rationale given above does not explain it.
So you need to come up with a more convincing rationale.

But I have some comments on the details of your change anyway.

> library/debugger_interface.m: 
> 	Split 'current' requests into 'current_var' and 'current_slots';
> 	'current_var' requests for live variables and 'current_slots' for the
> 	other slots.

I think it should be named `current_vars' not `current_var'.

> -			case MR_REQUEST_CURRENT:
> +
> +			case MR_REQUEST_CURRENT_VAR:
>  				if (MR_debug_socket) {
>  					fprintf(stderr, "\nMercury runtime: "
>  						"REQUEST_CURRENT\n");

s/REQUEST_CURRENT/MR_REQUEST_CURRENT_VAR/

And also s/VAR/VARS/ here and elsewhere.

> +			case MR_REQUEST_CURRENT_SLOTS:
> +				if (MR_debug_socket) {
> +					fprintf(stderr, "\nMercury runtime: "
> +						"REQUEST_CURRENT\n");

s/REQUEST_CURRENT/MR_REQUEST_CURRENT_SLOTS/

> +				MR_output_current_slots(layout, 
> +							port, 
> +							seqno, 
> +							depth, 
> +							path);

How about writing things horizontally rather than vertically?

				MR_output_current_slots(layout, port, seqno, 
							depth, path);

>  static void
> -MR_output_current(const MR_Stack_Layout_Label *layout,
> -	MR_trace_port port, Unsigned seqno, Unsigned depth,
> -	Word var_list,
> -	const char *path, Word current_request)
> +MR_output_current_slots(const MR_Stack_Layout_Label *layout,
> +	MR_trace_port port, int seqno, int depth, const char *path)

Why are the types here `int' instead of `Unsigned'?

> +/*
> +**
> +** This function returns the list of the internal names of currently live
> +** variables.
> +**
> +*/

Please use our standard commenting layout:

	/*
	** ...
	** ...
	*/

not
	/*
	**
	** ...
	** ...
	**
	*/

> +++ mercury_init.h	1998/07/24 15:25:52
> +void	ML_DI_output_current_slots(Integer, Integer, Integer, Word, String,
> +		String, Integer, Integer, Integer, String, Word);
> +		/* normally ML_DI_output_current_slots (output_current_slots/13) */

s/13/11/ ?

> +++ mercury_wrapper.h	1998/07/24 15:29:13
> +void	(*MR_DI_output_current_slots)(Integer, Integer, Integer, Word, String,
> +		String, Integer, Integer, Integer, String, Word);
> +		/* normally ML_DI_output_current_slots (output_current_slots/13) */

Ditto.

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