for review: Declarative debugger front end

[Lee Naish]

Sorry this review took me a while - too much teaching.

Mark Anthony BROWN <dougl at cs.mu.OZ.AU> writes:

> ** back end is an extension to the internal debugger which collects
> ** related trace events and builds them into an Evaluation Dependency
> ** Tree (EDT).  Once built, the EDT is passed to the front end where it can
>+** be analysed to find bugs.  The front end is implemented in
>+** browse/declarative_debugger.m.

A comment on the key interfaces (evaluation_tree, edt_node) would be
nice, and a comment about how these data structures could possibly be
build in different ways in the future.

>+		** MR_edt_{min,max}_depth.  These events are either
>+		** irrelevant, or else implicitly represented in the
>+		** structure being built.

Additional comments somewhere would be nice (eg this implicit
representation).  Maybe they existed already and are not in the diff?

> ** Each node in an EDT has a tag to denote its type.  At the moment
> ** the only type of analysis is wrong answer analysis, so the tag

Worth mentioning future existence of missing answer analysis?

> :- type edt_node
>	--->	wrong_answer(string, list(univ)).

As above.  More comments would be nice...

> :- type evaluation_tree == c_pointer.

> :- pragma c_code(edt_first_child(Parent::in, Child::out),

Do we really need C gunk here?  I can understand the horrible gunk
associated with converting low level trace stuff to a tree, but why
can't it be a nice Mercury tree?  It would be nice to be able to define
a Mercury tree type which could be constructed by awful C code or nicely
transformed Mercury source, allowing for data structure sharing instead
of copying, for example.  At the very least I would expect some comments
apologising for/explaining the design...

A possible answer to the objection above is that edt_first_child etc are
just the implementation of a well defined ADT.  That would be a good
answer if the ADT was properly documented and the interface and
implementation were properly separated.  However, edt_children and
edt_root (a nice abstract interface) are defined at the same level as
edt_first_child and there are not even any comments on these procedures.

> :- type declarative_bug
>	--->	unknown
>	;	wrong(evaluation_tree).

Add % EDT whose root node is buggy...

>	% This section implements the "oracle", which really just asks
>	% the user directly and returns their response.

> :- pred query_oracle(edt_node, bool, io__state, io__state).
> :- mode query_oracle(in, out, di, uo) is det.

Its worth saying that eventually an oracle database will be passed
around.  In fact I think its worth defining the right interfaces which
allow for this even if its not currently used (you can pass around a
dummy database which is never actually used - then future version will
be just changing the implementation of predicates with well defined
interfaces.  You will probably have to add an extra couple of arguments
to a bunch of preds, but the basic structure should remain the same (the
use of io__state has forced you to use the "right" structure anyway).

I think the use of type bool here will have to be changed eventually
also.  We may want to implement the three valued scheme and/or "don't
know" answers (see below).  I think introducing a new type, eg
edt_truth, is warranted.

> :- type debugger_command
>	--->	yes
>	;	no
>	;	browse
>	;	tree
>	;	help
>	;	unknown.

I suggest unknown be replaced by invalid/unrecognised/...
Eventually it would be nice for users to be able to respond "don't know"
to questions and then "unknown" will get confusing.

	lee