[m-rev.] for review: atoms in the debugger

[Zoltan Somogyi]

On 09-Jan-2002, Mark Brown <dougl at cs.mu.OZ.AU> wrote:
> On 31-Dec-2001, Zoltan Somogyi <zs at cs.mu.OZ.AU> wrote:
> > For review by Mark.

Mark, please delete from your reply the parts of the diff that you are not
commenting on. Including the whole diff makes it much harder to see your
comments buried in within the original diff.

> > +:- pred browser_term_to_string_list(list(univ)::in, int::in, int::in, int::out,
> > +	int::in, int::in, list(string)::out) is det.
> 
> This is a misnomer, since the predicate works on univs not browser_terms.
> I don't see a need to change the name of this predicate.

I settled on the name args_to_string_list.

> > +:- pred browser_term_to_string_verbose_list(list(univ)::in, int::in, int::in,
> > +	int::in, int::out, int::in, int::in, frame::out) is det.
> 
> See above comment for term_to_string_list.

This is now args_to_string_verbose_list.

> > +		SubBrowserTerm = plain_term(SubUniv)
> > +	;
> > +		BrowserTerm = synthetic_term(_FunctorString, Args, _IsFunc),
> > +		(
> > +			Path = [],
> > +			SubBrowserTerm = BrowserTerm
> > +		;
> > +			Path = [Dir | _Dirs],
> 
> Why is the rest of the path ignored?  There should be a call to
> deref_subterm_2/3 somewhere in here which processes the rest of the path.

Thanks for spotting that bug.

> > +:- type browser_term
> > +	--->	plain_term(
> > +			univ		% We are browsing a plain term.
> > +		)
> > +	;	synthetic_term(
> > +			string,		% We are browsing a synthetic term,
> > +					% such as a predicate name applied to
> > +					% a list of arguments. The string says
> > +					% what we should print as the functor.
> > +			list(univ),
> > +			bool
> > +		).
> > +
> 
> These last two fields should also be documented.
> 
> Come to think of it, a better design might be to separate the return value
> from the other arguments.  That is, have the second field be the list of
> arguments (without any return value), and the third field would be of type
> maybe(univ), which holds the return value iff there is one.  This would
> abstract away the fact that we make the return value the _last_ argument
> in the underlying predicate, which is an implementation detail that the
> users shouldn't have to think about.  I realise this would involve a
> fairly extensive change to the current diff, but it would make much of the
> rest of the code significantly easier to understand and maintain, IMHO.
> 
> I don't mind if you don't follow this suggestion right now.  But I think
> you should at least consider changing the type to something like what I
> just described.

I have followed your suggestion. However, the interface to the C code in the
trace directory still follows the old scheme, since the new scheme would
require C code to construct terms of the form yes(Univ). At the moment we don't
have macros to do that, and adding such macros would be a backwards
compatibility burden. We convert to your suggested representation as soon
as control enters Mercury code.

> > +		BrowserTerm = synthetic_term(Functor, Args, IsFunc),
> > +		string__length(Functor, FunctorSize),
> > +		list__length(Args, Arity),
> > +		(
> > +			IsFunc = yes,
> > +			PrincipalSize = FunctorSize + 1 + Arity * 2
> > +		;
> > +			IsFunc = no,
> > +			PrincipalSize = FunctorSize + Arity * 2
> > +		),
> 
> The variable Arity is mis-named; it should be something like NumArgs,
> or PredArity as Fergus suggested.

After the change in representation, it is not misleading any more, and +1
becomes +3.

> > +deconstruct_browser_term(BrowserTerm, Functor, Arity, Args, IsFunc) :-
> > +	(
> > +		BrowserTerm = plain_term(Univ),
> > +		deconstruct(univ_value(Univ), Functor, Arity, Args),
> > +		IsFunc = no
> > +	;
> > +		BrowserTerm = synthetic_term(Functor, Args, IsFunc),
> > +		list__length(Args, Arity)
> > +	).
> > +
> > +limited_deconstruct_browser_term(BrowserTerm, Limit, Functor, Arity, Args,
> > +		IsFunc) :-
> > +	(
> > +		BrowserTerm = plain_term(Univ),
> > +		limited_deconstruct(univ_value(Univ), Limit,
> > +			Functor, Arity, Args),
> > +		IsFunc = no
> > +	;
> > +		BrowserTerm = synthetic_term(Functor, Args, IsFunc),
> > +		list__length(Args, Arity)
> > +	).
> > +
> > +functor_browser_term(BrowserTerm, Functor, Arity, IsFunc) :-
> > +	(
> > +		BrowserTerm = plain_term(Univ),
> > +		functor(univ_value(Univ), Functor, Arity),
> > +		IsFunc = no
> > +	;
> > +		BrowserTerm = synthetic_term(Functor, Args, IsFunc),
> > +		list__length(Args, Arity)
> > +	).
> 
> These predicates calculate the arity incorrectly for functions, unless
> by "arity" you mean the arity of the underlying predicate, which I don't
> think you should.

Again, this problem goes away with the new representation. The last arguments
of deconstruct_browser_term and limited_deconstruct_browser_term turn into
maybe(univ)s.

I have done everything else you suggested as well. An updated diff will follow
after I bootcheck my changes.

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
post:  mercury-reviews at cs.mu.oz.au
administrative address: owner-mercury-reviews at cs.mu.oz.au
unsubscribe: Address: mercury-reviews-request at cs.mu.oz.au Message: unsubscribe
subscribe:   Address: mercury-reviews-request at cs.mu.oz.au Message: subscribe
--------------------------------------------------------------------------