updated diff for trace-based primitive debugger

[Fergus Henderson]

On 03-Feb-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> Fergus wrote:
> > On 02-Feb-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> > > 
> > > +typedef	struct MR_stack_layout_shape_struct {
> > > +	Word			*MR_stack_layout_shape_type;
> > > +	Word			MR_stack_layout_shape_inst;
> > > +} MR_stack_layout_shape;
> > 
> > Any particular reason for the long field names?
> 
> I would like to keep the field names unique, and I would like them to
> reflect the struct they are part of (navigating between these structures
> is hard enough without being confused about such things).

Hmm, when I see `foo.bar', if I want to know which structure `bar' is
a part of then I usually find it pretty easy to just go to the type
declaration for `foo'.  The long field names can make code difficult
to read, e.g. forcing line wraps after `.' or `->' operators:

	MR_stack_layout_shape *layout;
	...
	if (whatever) {
		MR_do_something(
			layout->MR_stack_layout_shape_type.
					MR_stack_layout_type_module,
			layout->MR_stack_layout_shape_type.
					MR_stack_layout_type_name,
			layout->MR_stack_layout_shape_type.
					MR_stack_layout_type_arity
		);
	}

instead of say
		
	MR_stack_layout_shape *layout;
	...
	if (whatever) {
		MR_do_something(layout->MR_type.MR_module,
				layout->MR_type.MR_name,
				layout->MR_type.MR_arity);
	}

> How about picking
> an abbreviation for each struct type and using that as a prefix? E.g.
> MR_sls_type and MR_sls_inst for the ones above (sls = stack layout shape),
> MR_sle_* for the fields of the per-entry struct, MR_sll_* for the per-label
> struct, and MR_slv_* for the per-var struct?

Hmm, I hate cryptic abbreviations like this.

> > > +		case MR_DETISM_CCMULTI:
> > > +			fprintf(stderr, "CCM  ");
> > > +			break;
> > 
> > These abbreviations are cryptic.
> > I suggest increasing the field width from 3 to 9 and
> > using the same names as in the Mercury language.
> > (i.e. `det', `semidet', `nondet', `multi',
> > `failure', `cc_nondet', `cc_multi', and `erroneous').
> 
> There is only so much space per line. I would like to use as much of the line
> as possible for displaying the qualified procedure name, since it can be
> quite long. Would DET, SEMI, NON, MUL, ERR, FAIL, CNON and CMUL do?

The first 6 are ok, but I'd much prefer CCNON and CCMUL.

> > Why use call_engine() rather than the commented out code?
> 
> If we export io__print as a C function and call it as in the commented out
> code above, which is what I did originally, the callback from C to Mercury
> clobbers part of the C stack. Although I know where this happens, I don't
> know why. Fergus and I should sit down together to find out.

OK, for the moment please document this with an XXX comment.

> > > +int
> > > +MR_trace_get_cmd(void)
> > > +{
> > > +	int	cmd;
> > > +	int	c;
> > > +
> > > +	cmd = getchar();	/* read the trace command */
> > > +
> > > +	/* skip the rest of the line */
> > > +	c = cmd;
> > > +	while (c != EOF && c != '\n')
> > > +		c = getchar();
> > > +
> > > +	return cmd;
> > >  }
> > 
> > What happened to the enum that you used for commands?
> > I think that was a good idea.
> 
> This function just gets a char and ignores the rest of the line. Finding
> out whether the char is a valid command is the caller's job; there is no
> point in duplicating it. Also, some commands the user can give (e.g. "p")
> don't have an enum value.

OK.

> > Also, single-character command names are nice for interactive use,
> > but it would be better if the tracer also had long name equivalents.
> 
> There a lot of other things the trace debugger should have, but one step
> at a time.

Fair enough.

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