[m-rev.] for post-commit review: goal ids

[Paul Bone]

On Mon, Jan 03, 2011 at 05:37:50PM +1100, Zoltan Somogyi wrote:
> On 03-Jan-2011, Paul Bone <pbone at csse.unimelb.edu.au> wrote:
> > > that tells us the parent goal of ever subgoal, with the obvious exception
> > 
> >     s/ever/every/
> 
> There is no point in reviewing the Log message after the change has been
> committed, since there is no practical way to update CVS's records of it.

Whoops :-)

> > >      CallCode = from_list([
> > >          llds_instr(livevals(LiveVals), ""),
> > >          llds_instr(llcall(CodeAddr, code_label(ReturnLabel), ReturnLiveLvalues,
> > > -            Context, GoalPath, CallModel), "Setup and call"),
> > > +            Context, MaybeGoalPath, CallModel), "Setup and call"),
> > >          llds_instr(label(ReturnLabel), "Continuation label")
> > >      ]),
> > >  
> > 
> > In what cases will !.CI not have a containing goal map?
> 
> If the procedure is compiled with no execution tracing, since then
> we don't need goal paths at all. This is documented in the comment for llcall
> in llds.m.
> 
> > Is it bad if llds_instr is not provided a goal path?
> 
> No.

Okay.

> > > +fill_conj_id_slots(_, _, _, !GoalNum, !ContainingGoalMap, [], []).
> > > +fill_conj_id_slots(SlotInfo, GoalId, N0, !GoalNum, !ContainingGoalMap,
> > > +        [Goal0 | Goals0], [Goal | Goals]) :-
> > > +    N1 = N0 + 1,
> > > +    ContainingGoal = containing_goal(GoalId, step_conj(N1)),
> > > +    fill_goal_id_slots(SlotInfo, ContainingGoal, !GoalNum, !ContainingGoalMap,
> > > +        Goal0, Goal),
> > > +    fill_conj_id_slots(SlotInfo, GoalId, N1, !GoalNum, !ContainingGoalMap,
> > > +        Goals0, Goals).
> > > +
> > 
> > Is it clearer to make the step step_conj(N0) rather than N1, and call this
> > predicate with 1 for the parameter N0?
> 
> I think it is equally clear either way.

That's fine. This is a subjective judgement.

> > > +:- type program_point
> > > +    --->    pp(
> > > +                pp_context  :: term.context,
> > > +                pp_id       :: reverse_goal_path
> > >              ).
> > 
> > Shound the field name still be pp_path if this is a goal path rather
> > than a goal id?
> 
> Whatever the type, it still identifies a program point, and in the future,
> it SHOULD be a goal_id.

I agree with pp_id if it should be a goal_id in the future.

> > > Index: deep_profiler/coverage.m
> > > ===================================================================
> > > RCS file: /home/mercury/mercury1/repository/mercury/deep_profiler/coverage.m,v
> > > retrieving revision 1.10
> > > diff -u -r1.10 coverage.m
> > > --- deep_profiler/coverage.m	15 Dec 2010 06:30:33 -0000	1.10
> > > +++ deep_profiler/coverage.m	17 Dec 2010 02:41:26 -0000
> > > @@ -431,19 +440,20 @@
> > >      % conjunction.  Each goal also has it's own coverage.
> > >      %
> > >  :- pred conj_annotate_coverage_2(coverage_reference_info::in,
> > > -    goal_path::in, int::in, coverage_before::in, coverage_after::out,
> > > +    list(goal_path_step)::in, int::in,
> > > +    coverage_before::in, coverage_after::out,
> > >      list(goal_rep(unit))::in, list(goal_rep(coverage_info))::out) is det.
> > 
> > We shoult make the list of goal path steps here an actual goal_path
> > type since this type is no-longer abstract.  This is also true for a
> > number of places below.
> 
> If you did, you would need to break the abstraction barrier all the time,
> so I don't think you would gain anything, and the cost in clarity would
> be significant.

This is true without functions such as remove first and cons for goal paths.
But I belive that the list syntax is easier to read than these functions.

> > > @@ -4272,10 +4272,10 @@
> > >      CallSiteDesc = call_site_desc(CSSPtr, _ContainerPSPtr,
> > >          _FileName, _LineNumber, _ModuleName,
> > >          _UnQualRefinedName, _QualRefinedName,
> > > -        SlotNumber, GoalPath, _MaybeCallee),
> > > +        SlotNumber, RevGoalPath, _MaybeCallee),
> > >      RefinedName = call_site_desc_get_caller_refined_id(MaybeCurModuleName,
> > >          ModuleQual, CallSiteDesc),
> > > -    GoalPathStr = goal_path_to_string(GoalPath),
> > > +    GoalPathStr = rev_goal_path_to_string(RevGoalPath),
> > >      string.format("%s @ %s #%d",
> > >          [s(RefinedName), s(GoalPathStr), i(SlotNumber)], Name),
> > >      Cmd = deep_cmd_dump_call_site_static(CSSPtr),
> > 
> > For printing reports we should probably print goal paths in forward
> > order.  That is the path from the root to the goal in question.
> 
> We do. rev_goal_path_to_string takes a reverse list, but the string it returns
> is in forward order. The string forms of goal paths should ALWAYS be in forward
> order. If you find any that aren't, that is a bug.

Good :-)  I like this behaviour.

> > > -        GoalPath \= ConjGoalPath,
> > > -        goal_path_inside(ConjGoalPath, GoalPath, RelativePath),
> > > -        goal_path_consable(RelativePath, RelativePathCons),
> > > -        goal_path_consable_remove_first(RelativePathCons, Step, _),
> > > +        % XXX zs: I am not confident of the update for goal path
> > > +        % representations.
> > > +        RevGoalPath \= RevConjGoalPath,
> > > +        rev_goal_path_inside(RevConjGoalPath, RevGoalPath, RevRelativePath),
> > > +        RevRelativePath = rgp(RevRelativePathSteps),
> > > +        list.last(RevRelativePathSteps, Step),
> > >          Step = step_conj(ConjNum),
> > >          ConjNum > FirstConjunct,
> > >          ConjNum =< FirstConjunct + Length
> > 
> > This is correct.  The XXX can be removed.
> 
> Done.
> 
> Thanks for the review.

No problems, I just had to be patient :-)  The diff was roughly 20,000 LoC.

I have my new glasses now including a pair of reading glasses, they make
reading diffs much faster as it's easier for me to scan text especially to
compare + and - lines in diffs.  This is because the reading glasses offer me a
wider field of view.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 490 bytes
Desc: Digital signature
URL: <http://lists.mercurylang.org/archives/reviews/attachments/20110104/19dd9fbb/attachment.sig>