[m-rev.] For review: allow declarative debugger to search upward from initial symptom

[Ian MacLarty]

On Mon, Dec 13, 2004 at 03:18:15PM +1100, Julien Fischer wrote:
> > -% Copyright (C) 1999-2004 The University of Melbourne.
> > +% Copyright (C) 1999-2003 The University of Melbourne.
> The year is wrong here.
> 

Fixed.

> > +	%
> > +	% If there's no root yet (because the oracle hasn't asserted any nodes
> > +	% are erroneous yet, then use the topmost suspect as a starting point.
> > +	%
> There's unmatched parenthesis in that comment.
> 

Matched.

> > +			% can't return (like if we come across an erroneous
> > +			% node where the sub-term is an input).
> > +			%
> Make that last bit "For example, if we come across ..."
> 

Okay.

> > +:- pred diagnoser_require_supertree(diagnoser_response(trace_node_id),
> > +	event_number, sequence_number).
> > +:- mode diagnoser_require_supertree(in, out, out) is semidet.
> > +
> You can use predmode syntax there.
> 

There are a lot of places where predmode syntax needs to be used in this file,
I am planning to do a clean up of all the outdated syntax sometime soon, so
I'll fix that then.

> > Index: browser/declarative_edt.m
> 
> >
> >  suspect_correct_or_inadmissible(SearchSpace, SuspectId) :-
> >  	lookup_suspect(SearchSpace, SuspectId, Suspect),
> > @@ -523,7 +603,7 @@
> >
> >  	% Succeeds if we haven't got an answer from the oracle about this
> >  	% suspect, and haven't been able to infer anything about this suspect
> > -	% from other oracle answers?
> > +	% from other oracle answers.
> >  	%
> >  :- pred suspect_is_questionable(search_space(T)::in, suspect_id::in)
> >  	is semidet.
> > @@ -593,59 +673,125 @@
> >  in_buggy_subtree(in_erroneous_subtree_complement, no).
> >  in_buggy_subtree(unknown, yes).
> >
> > -
> > -	% Should the suspect's status be propogated to it's children when the
> > -	% children are added to the search space?
> > +	% What status should be assigned to children of a node with the given
> > +	% status, when the children are being added to the search space?
> >  	%
> I would reword that comment so that it just describes what the function
> does.
> 

I've reworded it to:

	% Return the status that should be assigned to children of a suspect 
	% with the given status, when the children are being added to the
	% search space.
	%

> > +new_child_status(unknown) = unknown.
> > +
> > +	% What status should be assigned to the parent of a node with the given
> > +	% status, when the parent is being added to the search space?
> > +	%
> Likewise here.
> 

	% Return the status that should be assigned to the parent of a suspect
	% with the given status, when the parent is being added to the search
	% space.
	%
	
> > +	% A version of trickle_status which doesn't return Leaves.
> > +	%
> s/Leaves/leaves/.
> 

Fixed.

> > +:- pred trickle_status(suspect_status::in, list(suspect_status)::in,
> > +	suspect_id::in, search_space(T)::in, search_space(T)::out) is det.
> > +
> > +trickle_status(Status, StopStatusSet, SuspectId, !SearchSpace) :-
> > +	trickle_status(Status, StopStatusSet, SuspectId, _,
> > +		!SearchSpace).
> >
> I'm not sure that using terms like trickle and perculate is very
> clear - propagate_upwards and propgate_downwards might be more
> straightforward (albeit less colourful).
> 

I've changed the names to propagate_status_upwards and
propagate_status_downwards.

> > -trickle_status(Status, SuspectId, !SearchSpace) :-
> > +:- pred trickle_status(suspect_status::in, list(suspect_status)::in,
> > +	suspect_id::in, list(suspect_id)::in, list(suspect_id)::out,
> > +	search_space(T)::in, search_space(T)::out) is det.
> > +
> > +trickle_status(Status, StopStatusSet, SuspectId, !Leaves, !SearchSpace) :-
> >  	lookup_suspect(!.SearchSpace, SuspectId, Suspect),
> >  	(
> > -		Suspect ^ status \= Status
> > +		\+ member(Suspect ^ status, StopStatusSet)
> >  	->
> >  		map.set(!.SearchSpace ^ store, SuspectId,
> >  			Suspect ^ status := Status, Store),
> >  		!:SearchSpace = !.SearchSpace ^ store := Store,
> >  		(
> >  			Suspect ^ children = yes(Children),
> > -			list.foldl(trickle_status(Status), Children,
> > +			list.foldl2(trickle_status(Status, StopStatusSet),
> > +				Children, !Leaves, !SearchSpace)
> > +		;
> > +			Suspect ^ children = no,
> > +			adjust_unexplored_leaves(yes(Suspect ^ status), Status,
> >  				!SearchSpace)
> > +		),
> > +		adjust_unknown_count(yes(Suspect ^ status), Status,
> > +			!SearchSpace)
> > +	;
> > +		!:Leaves = [SuspectId | !.Leaves]
> You could also write that as
> 
> 		list.cons(SuspectId, !Leaves)

True.

> 
> 
> > +	% Increments or decremenets the unknown suspect count after a status
> s/decremenets/decrements/
> 

Fixed.

> > +	% change.  The 1st argument should be the previous status of the
> > +	% changed suspect or no if a new suspect is being added and the 2nd
> > +	% argument should be the suspect's new status.
> > +	%
> Use first and second rather than 1st and 2nd.
> 

Have changed 1st, 2nd and 3rd throughout.

> > +	% Increments or decremenets the unexplored leaves count after a status
> > +	% change.  The 1st argument should be the previous status of the
> > +	% changed suspect or no if a new suspect is being added and the 2nd
> > +	% argument should be the suspect's new status.  The changed suspect
> > +	% should be a leaf node (i.e. have its children field set to no).
> > +	%
> There are some copy and paste errors from above here.
> 

Fixed.

> > +	% Add the given EDT node to the top of the search space.  The given
> > +	% node should be the parent of the current topmost node in the search
> s/the parent/a parent/
> 

Yep.

> > +			%
> > +			% One of the children of the new top most node will be
> s/top most/topmost/

Fixed.

> > +
> > +			%
> > +			% Adjust the unexplored leaves count since the new top
> > +			% most node was added with no children.
> topnost again
> 

Done.

> >  	/*
> > -	** If this event is an interface event then increase or decrease
> > -	** the EDT depth appropriately.
> > +	** Filter out events for compiler generated procedures.
> > +	** XXX Compiler generated unify procedures should be included
> > +	** in the annotated trace so that sub-term dependencies can be
> > +	** tracked through them.
> > +	*/
> The comment should mention that uncommenting the code below is necessary
> for this to work.
> 

I have now mentioned this.

> > +				** genertaed EDT, so we don't return here.
> > +				*/
> s/genertaed/generated/
> 

Fixed.

> > +	** Set this to the preceding node, so the new explicit tree's parent is
> >  	** resolved correcly.
> s/correcly /correctly/
> 

Fixed.

> > @@ -5335,8 +5335,9 @@
> >  	MR_Code **jumpaddr)
> >  {
> >  	MR_trace_decl_assume_all_io_is_tabled = MR_FALSE;
> > +	MR_edt_depth_step_size = 3;
> Why 3?
> 

It has been 3 since I started work on the debugger.  The choice is pretty
arbitrary, although the benefits of such a low default depth is that the
generation of new parts of the annotated trace is excercised during testing and
we probably won't run out of memory for wide trees.

The plan is to eventually dynamically adjust this number based on the shape of
the tree and how much memory is available.  (I think this is mentioned in
mercury_trace_declarative.c).

> >  	if (! MR_trace_options_dd(&MR_trace_decl_assume_all_io_is_tabled,
> > -		&words, &word_count, "dd", "dd"))
> > +		&MR_edt_depth_step_size, &words, &word_count, "dd", "dd"))
> >  	{
> >  		; /* the usage message has already been printed */
> >  	} else if (word_count == 1) {
> > @@ -5369,8 +5370,9 @@
> >  	const char	*filename;
> >
> >  	MR_trace_decl_assume_all_io_is_tabled = MR_FALSE;
> > +	MR_edt_depth_step_size = 3;
> and here?  I think you should use a symbolic constant.
> 


I've added the following to mercury_trace_declarative.h and used this 
constant in both places you pointed out.

/*
** The initial depth step size.  The choice of 3 is quite arbitrary, but
** it does mean the code which generates new portions of the annotated trace
** is exercised more during testing.
*/

#define MR_TRACE_DECL_INITIAL_DEPTH	3


Thanks for the comments,

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