your mail

[Fergus Henderson]

On 09-Feb-1999, Erwan Jahier <Erwan.Jahier at irisa.fr> wrote:
> This change implement the retry command for the external debugger.

That looks good -- thanks Erwan.
I do have a few comments, though.

> trace/mercury_trace.[ch]:
> trace/mercury_trace_internal.[ch]:
> 	Move MR_trace_retry() and MR_trace_find_input_arg() from 
> 	mercury_trace_internal.c to mercury_trace.c since they are also needed
> 	in the external debugger.
> 
> 	MR_trace_retry() now returns TRUE if the retry proceeded correctly 
> 	and FALSE otherwise.

It would probably be better for it to return a `char *' pointer which
is either NULL if the retry proceeds correctly or an error message
if it fails.

> +extern bool
> +MR_trace_retry(const MR_Stack_Layout_Label *this_label, Word *saved_regs,
> +	MR_Event_Details *event_details, int seqno, int depth,
> +	int *max_mr_num, Code **jumpaddr)
> +{

`extern' linkage is the default for functions,
and using it on function definitions seems misleading,
since it seems to imply "defined somewhere else", when
in fact the function is defined right here.
So I recommend not using an explicit `extern' on function definitions.

...
> +	if (call_label->MR_sll_var_count < 0) {
> +		printf("Cannot perform retry because information about "
> +			"the input arguments is not available.\n");
> +		return FALSE;
> +	}

This should not use printf().
For the internal debugger, it should use

	fflush(mdb_out);
	fprintf(mdb_err, ...)

instead.  For the external debugger, printing anything at
all is probably wrong.  Better to just return the error
message, as I suggested above.

> +		if (! succeeded) {
> +			printf("Cannot perform retry because the values of "
> +				"some input arguments are missing.\n");
> +			return FALSE;

Likewise here.

> +extern Word
> +MR_trace_find_input_arg(const MR_Stack_Layout_Label *label, Word *saved_regs,
> +	const char *name, bool *succeeded)
> +{

As above, please don't use `extern' on function definitions.

mercury_trace_external.c:
> -		len = SUN_LEN(&unix_address);
> -	} else {
> +		len = sizeof(&unix_address);
> +	}  else {

That change isn't documented in the log message.

The additional space before the "else" is probably a mistake.

I don't remember what SUN_LEN() was supposed to be --
it may be worth checking Stevens, "Advanced Programming in
the UNIX environment", which is probably where it came from --
but the use of `sizeof(&unix_address)' looks to me like it could
well wrong; I think maybe `sizeof(unix_address)' might be
what was intended.

>  MR_trace_event_external(MR_Trace_Cmd_Info *cmd, 
...
> +	Code		*jumpaddr= NULL;

I suggest s/=/ =/

> +				if (MR_trace_retry(layout, saved_regs, 
> +					&event_details, seqno, depth, 
> +					max_mr_num, &jumpaddr)) 
> +
> +				  {
> +				    MR_send_message_to_socket("ok");
> +				    cmd->MR_trace_cmd = MR_CMD_GOTO;
> +				    cmd->MR_trace_stop_event = 
> +					MR_trace_event_number + 1;
> +				    return jumpaddr;
> +				  }
> +				else
> +				  {
> +				    MR_send_message_to_socket("retry_failed");
> +				  }
> +				break;

Please use our normal conventions for indentation and placement
of curly braces:

	if (...
		...
		...)
	{
		...
	} else {
		...
	}

rather than

	if (...
		...
		...)
	  {
	    ...
	  }
	else
	  {
	    ...
	  }

Apart from that, your change looks fine.
But I would like to see another diff when you have addressed
those issues.

-- 
Fergus Henderson <fjh at cs.mu.oz.au>  |  "Binaries may die
WWW: <http://www.cs.mu.oz.au/~fjh>  |   but source code lives forever"
PGP: finger fjh at 128.250.37.3        |     -- leaked Microsoft memo.