[m-rev.] for review: cleanup mercury_trace_declarative.c

[Ian MacLarty]

On Sun, Apr 10, 2005 at 05:51:54PM +1000, Julien Fischer wrote:
> 
> On Sun, 10 Apr 2005, Ian MacLarty wrote:
> 
> > In this diff I have also included compiler generated unifications in the
> > annotated trace.  The declarative debugger wont ask questions about these
> > because they are automatically trusted, but they are needed in case a subterm
> > needs to be tracked though a complicated unification.
> >
> > I was unable, however to create a test case that caused a __Unify__ predicate
> > to be generated which produced output.  Are all compiler generated unifications
> > just test unifications?
> 
> I'm pretty sure that all of the compiler generated unification
> predicates are (in, in). (The code that generates them is in
> compiler/unify_proc.m).
> 

In this case I will not include compiler generated unifications in the 
annotated trace, since they cannot be responsible for generating a subterm.

> >  If not could someone give me an example of a
> > complicated unification which produces output and for which the compiler
> > generates a seperate procedure?  All the complicated unifications in the
> > tests directories seem to be test-unifications (i.e. unifications that produce
> > no output).
> >
> > Cheers,
> > Ian.
> >
> > Estimated hours taken: 5
> > Branches: main
> >
> > Clean up trace/mercury_trace_declarative.c by breaking MR_trace_decl_debug into
> > several functions.  This function was getting very big and difficult to
> > maintain.
> >
> > trace/mercury_trace_declarative.c:
> > 	Break MR_trace_decl_debug into MR_trace_edt_build_sanity_check, which
> > 	checks that we haven't passed the last event for the portion of the
> > 	trace being constructed; MR_trace_include_event, which decides if an
> > 	event should be included in the annotated trace and resets the depth
> > 	if the event is at the top of an area to be materialized;
> > 	MR_trace_calculate_event_depth, which calculates the depth in the EDT
> > 	of the current event and sets the global MR_edt_depth appropriately;
> > 	And MR_trace_construct_node, which constructs a node in the annotated
> > 	trace.
> >
> That would read better as:
> 
> 	Decompose MR_trace_decl_debug into smaller functions:
> 	MR_trace_edt_build_sanity_check, ... etc
> 

Done.

> > 	Uncomment the code that includes compiler generated unifications in the
> > 	annotated trace.
> 
> The log message should mention why it is ok to uncomment this code now.
> 

This code has now been removed.

> > +static	MR_bool		MR_trace_include_event(const MR_Proc_Layout *entry,
> > +				MR_Event_Info *event_info,
> > +				MR_Code **jumpaddr);
> > +static	MR_Unsigned	MR_trace_calculate_event_depth(
> > +				MR_Event_Info *event_info);
> > +static	void		MR_trace_construct_node(MR_Event_Info *event_info);
> >
> >  MR_bool MR_trace_decl_assume_all_io_is_tabled = MR_FALSE;
> >
> > @@ -339,6 +346,23 @@
> >  MR_bool		MR_trace_decl_in_dd_dd_mode = MR_FALSE;
> >
> >  /*
> > +** We filter out events which are deeper than a certain
> Delete the word "certain".
> 

Okay.

> > +** limit given by MR_edt_max_depth.  These events are
> > +** implicitly represented in the structure being built.
> > +** Adding 1 to the depth limit ensures we get all the
> > +** interface events inside a call at the depth limit.
> > +** We need these to build the correct contour.
> > +** Note that interface events captured at max-depth + 1
> Is that max minus depth or max_depth?
> 

max dash depth.  I have changed it to "the maximum depth plus one".

> > +** will have their at-depth-limit flag set to no.  This
> > +** is not a problem, since the parent's flag will be yes,
> > +** so there is no chance that the analyser could ask for the
> > +** children of an interface event captured at max-depth + 1,
> > +** without the annotated trace being rebuilt from the parent
> > +** first.
> Make that last bit a separate sentence.  ie.
> 
> 	There is no chance that the analyser could ask ...
> 

Okay.

> 
> > +*/
> > +#define MR_TRACE_EVENT_TOO_DEEP(depth) depth > MR_edt_max_depth + 1
> > +
> You should parantheses around the body of that macro.
> 

Okay.

> > +
> > +	trace_suppress = entry->MR_sle_module_layout->MR_ml_suppressed_events;
> > +	if (trace_suppress != 0) {
> > +		/*
> > +		** We ignore events from modules that were not compiled
> > +		** with the necessary information.  Procedures in those
> > +		** modules are effectively assumed correct, so we give
> > +		** the user a warning.
> > +		*/
> That comment is a little unclear, perhaps the following:
> 
> 	We ignore events from modules that were not compiled with
> 	the necessary information.  In the absence of such information
> 	we have to asssume that procedures in those modules are correct.
> 	We issue a warning to let the user know that this has occurred.
> 

I think the original version is clearer.

> > +	MR_Event_Info *event_info, MR_Code **jumpaddr)
> > +{
> >  	/*
> >  	** Filter out events for compiler generated procedures.
> > -	** XXX Compiler generated unify procedures should be included
> > +	** Compiler generated unify procedures should be included
> >  	** in the annotated trace so that sub-term dependencies can be
> You should s/sub-term/subterm/ in the line above.

Done.

> 
> That looks ok otherwise.
> 

Cheers,

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