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

[Zoltan Somogyi]

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.

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

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

> > +    % We use the globals from the original module info, so that
> > +    module_info_get_globals(ModuleInfo0, Globals0),
> > +    globals.get_trace_level(Globals0, TraceLevel0),
> > +    ( given_trace_level_is_none(TraceLevel0) = no ->
> > +        fill_goal_id_slots_in_proc(ModuleInfo, ContainingGoalMap,
> > +            ProcInfo1, ProcInfo),
> > +        MaybeContainingGoalMap = yes(ContainingGoalMap)
> > +    ;
> > +        MaybeContainingGoalMap = no,
> > +        ProcInfo = ProcInfo1
> > +    ),
> 
> The comment above trails off.

I wrote it a long time ago, and have forgotten why that was required.
I will see whether I can reconstruct the reason.

> > +:- 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.

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

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

> > -format_cp_info(Num, coverage_point_info(Path, CPType), String) :-
> > -    goal_path_to_string(Path) = PathString,
> > +format_cp_info(Num, coverage_point_info(RevPath, CPType), String) :-
> > +    rev_goal_path_to_string(RevPath) = PathString,
> >      format("coverage_point[%d]: %s, %s",
> >          [i(Num), s(string(CPType)), s(PathString)], String).
> 
> As above.

Same answer.

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

Zoltan.
--------------------------------------------------------------------------
mercury-reviews mailing list
Post messages to:       mercury-reviews at csse.unimelb.edu.au
Administrative Queries: owner-mercury-reviews at csse.unimelb.edu.au
Subscriptions:          mercury-reviews-request at csse.unimelb.edu.au
--------------------------------------------------------------------------