[m-dev.] for review: new debugger command set (part 1 of 4)

[Fergus Henderson]

On 13-Oct-1998, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> 
> > stack_layout.m:
> > >  :- pred stack_layout__construct_proc_layout(label::in,
> > >  	determinism::in, int::in, maybe(int)::in, maybe(label)::in,
> > > -	stack_layout_info::in, stack_layout_info::out) is det.
> > > +	maybe(int)::in, stack_layout_info::in, stack_layout_info::out) is det.
> > 
> > Here -- and in lots of other places -- I think it would be clearer
> > if you used a typedef called say `maybe_from_full_slot' rather than
> > `maybe(int)', particularly if there was a comment next to the
> > definition of the type explaining its purpose.
> 
> I will keep it maybe(int) for now. I plan to add another slot later for Mark;
> I will follow your suggestion then.

OK.

> > > +		string__append_list([
> > > +			FillThreeSlots, "\n",
> > > +			"\t\t", RedoLayoutStr, " = (Word) (const Word *) &",
> > > +			LayoutAddrStr, ";"
> > 
> > Why the double cast?  Why not cast straight to `Word'?
> 

Hmm, no comment on this one?

> > > +** Group (2) fields have a different interpretation if the procedure is
> > > +** compiler-generated. You can test for this via the macro
> > > +** MR_ENTRY_LAYOUT_COMPILER_GENERATED.
> > 
> > What's the different interpretation?
> 
> It is documented in stack_layout.m.

OK, then could you please add a pointer to that documentation here.

> > > +#define	MR_ENTRY_LAYOUT_COMPILER_GENERATED(entry)		\
> > > +		((int) entry->MR_sle_pred_or_func > MR_FUNCTION)
> > 
> > Hmm, that code is a bit obfuscated -- a comment here would be helpful.
> > If special values are being used as sentinels, it would probably
> > be a good idea to make those special values part of the enum.
> 
> The "special values" are pointers, so they cannot be part of the data type.

Then could you please add a comment at the data type definition
and/or the field definition explaining this.

If the special values are pointers, then casting to `int' and using `>'
is not the right test.  Firstly, casting to `int' may truncate the pointer;
you should cast to an integer type of the same size as a pointer, e.g.
`Integer'.  Secondly, a pointer cast to a signed type may well be
negative, and thus not < MR_FUNCTION.  You should cast to an unsigned
type, e.g. `Unsigned'.

> > > +void
> > > +MR_print_proc_id(FILE *fp, const MR_Stack_Layout_Entry *entry_layout,
> > > +	const char *extra)
> > > +{
> > ...
> > > +	fprintf(fp, "%s %s:%s/%ld-%ld (%s)",
> > > +		entry_layout->MR_sle_pred_or_func == MR_PREDICATE ?
> > > +			"pred" : "func",
> > 
> > Will this incorrectly print "func" for compiler-generated procedures?
> 
> Yes.
...
> I will fix that, but not in this change.

You should at very least add an "XXX" comment as part of this change.

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