Re: [Info-gnuprologjava] Adding hooks for executing code when frames are being discarded

[Daniel Thomas]

Hello,

I have blocked out some time on Saturday for dealing with this.

Daniel

On Fri, 2012-03-02 at 07:56 +1300, Matt Lilley wrote:
> Hello,
> 
> I've attached a patch that fixes three problems with my original 
> submission (and some test cases to cover those, where possible: Because 
> the cleanup doesn't allow exceptions to be thrown, I don't think it's 
> possible to use the test harness to check at least one case, since the 
> harness relies on checking the state after the test has completed, 
> whereas we are interested in state which is immediately backtracked over 
> and lost...)
> 
> The first is that setup_call_catcher_cleanup/4 should not succeed if the 
> goal fails, unifying the Catcher to fail; it should unify the Catcher to 
> fail, call the cleanup, and then *fail*.
> The second, possibly more important fix, is that when backtracking into 
> the goal, we should not call the cleanup at all - only when backtracking 
> *out* of the goal because there are no more solutions. To do this, I 
> changed Interpreter.popBacktrackInfo() to skip over the cleanup frames 
> without cleaning them up. This certainly works, but I wonder if there 
> might be other places that need to also do this.
> Finally, when running the cleanup, we need to make sure that afterwards 
> the stack looks exactly like it did before the cleanup. To do this I 
> peek at the top frame on the stack and pop back to it after running the 
> cleanup.
> 
> Testing this in my application, it does a much better job than before - 
> I should've tested more thoroughly before the original submission!
> 
> Thanks again,
> Matt
> > Sorry for the months of delay. This has now been pushed with only minor
> > changes.
> > I decided against the throwing of PrologException in cleanup as this was
> > resulting in compile errors in several other places which needed to
> > handle this so for now I think it better to ignore exceptions in the
> > cleanup method. If it is important that they do get thrown then I can
> > reconsider that.
> > The code also got reformatted as I have eclipse set to vigorously
> > enforce GNU's code formatting guidelines.
> > I moved the unit test you added into extra as it is not testing a core
> > feature.
> >
> > Do you want me to spin up a 0.2.x branch release? Or let this form part
> > of the 0.3.0 release (which won't be until the autumn at the very
> > earliest)?
> >
> > Thank you,
> >
> > Daniel
> >
> > On Fri, 2011-12-16 at 11:56 +1300, Matt Lilley wrote:
> >> Hello,
> >>
> >> Sorry for the confusion; I had intended
> >> Predicate_setup_call_catcher_cleanup.java as an illustration of what
> >> could be achieved by adding cleanup information to the stack; I hadn't
> >> intended it to be included in the main codebase.
> >>
> >> Nonetheless, I've tidied it up significantly and adjusted the indent and
> >> coding style to more closely match yours. I've included a patch as a
> >> diff against 0.3.0 (Yes, I'd originally written it against 0.2.6). I
> >> made a slight modification to the way it's implemented as well;
> >> essentially keeping some extra backtrack frames with just the cleanup
> >> info in them. This avoids having to pop off a frame when we detect
> >> there's a choicepoint and push a modified frame; instead we just push an
> >> extra frame which has no effect if we undo it.
> >>
> >> I've also included a few tests after reverse engineering the testing
> >> format. Hopefully I guessed correctly. I should point out that even with
> >> a brand new clone of the git repository, some 26 tests fail, and I get
> >> predicate undef_pred/0 does not exist.
> >> predicate ^/2 does not exist.
> >> predicate ^/2 does not exist.
> >> printed to stderr. Is that expected?
> >>
> >> Let me know if you have any other suggestions for improvements to the
> >> feature.
> >>
> >> Regards,
> >>
> >> Matt
> >>> Hello,
> >>>
> >>> To add tests put something suitable into the inria suite in
> >>> test/inriasuite.
> >>> They can be run with:
> >>> gnu.prolog.test.GoalRunner --once inriasuite.pl run_all_tests
> >>>
> >>>
> >>> Predicate_setup_call_catcher_cleanup wants a copyright header and a
> >>> package. It also needs to be wired in via an entry in a .pro in
> >>> gnu.prolog.vm.buildins. I would prefer it in a patch rather than as the
> >>> single file as a patch has metadata about where it should go.
> >>>
> >>> Which version of gnuprologjava did you write
> >>> Predicate_setup_call_catcher_cleanup? 0.2.6? It isn't current head
> >>> anyway - which doesn't matter I can do a 0.2.7 release with this in and
> >>> port the code to the current newmaster.
> >>>
> >>> In terms of imports I prefer to avoid using foo.bar.* as when later I do
> >>> Ctrl-Shift-o in eclipse they will get expanded.
> >>>
> >>> When using stuff from args[] if you are only using a particular index
> >>> once then it is ok to not assign it to a variable with a meaningful name
> >>> but when using a particular index more than once it is much more
> >>> readable for them to be named by assigning to a variable at the
> >>> beginning. Probably also good to check that that args[] is the right
> >>> length (though with correct wiring up it should always get the right
> >>> number of arguments).
> >>>
> >>> When ignoring Exceptions a comment is required explaining why this is
> >>> sensible. The correct thing to do in this case would probably be to log
> >>> this at info or warning but we don't have a logging framework a TODO
> >>> would be helpful for when I add that in later.
> >>>
> >>>
> >>> BacktrackInfoWithCleanup needs its copyright header updating as it
> >>> doesn't have you on it and if should only have Constantine on it if you
> >>> are reusing code that Constantine wrote.
> >>> Similarly a TODO about logging the exception would be helpful.
> >>>
> >>> With those addressed and tests to prove it works and to prevent me
> >>> breaking it then that should be fine.
> >>>
> >>> Thank you,
> >>>
> >>> Daniel
> >>>
> >>> On Thu, 2011-12-15 at 11:18 +1300, Matt Lilley wrote:
> >>>> Hello,
> >>>>
> >>>> I've attached a patch that does the trick. I don't think it's too
> >>>> inefficient, though it does make popping frames a bit slower (I don't
> >>>> think there's any way you can avoid that if the task at hand is to add
> >>>> complexity to that very process). Also attached is an implementation of
> >>>> Predicate_setup_call_catcher_cleanup, the more general case of
> >>>> setup_call_cleanup/3. I'm not entirely sure how your testing framework
> >>>> works, but I can provide a prolog file as well if that would help.
> >>>>
> >>>> I'm happy to rework this if you have suggestions on how it could be
> >>>> better integrated; I'm obviously very new to your project!
> >>>>
> >>>> Thanks again,
> >>>> Matt
> >>>>
> >>>>> Hello,
> >>>>>
> >>>>> If you want to submit a patch to add that functionality then that would
> >>>>> be great, I have no problem with adding non-ISO features as long as ISO
> >>>>> features don't get broken and as long as it can be implemented cleanly.
> >>>>> Unfortunately I don't have time to work out how to write such a patch
> >>>>> myself at the moment but I do have time to review it. If you could add
> >>>>> some test cases to prove it works that would be great.
> >>>>>
> >>>>> If implementing it cleanly is not possible then we can look at what
> >>>>> could be changed to either make that possible or to allow a local hack.
> >>>>>
> >>>>> If you have any questions then please ask.
> >>>>>
> >>>>> Best regards,
> >>>>>
> >>>>> Daniel
> >>>>>
> >>>>>
> >>
> 
>