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

[Daniel Thomas]

Looks good, I have pushed this up, sorry for the delay.
It sounds like there could be other complexities of supporting this
which we haven't spotted yet but I don't think there is much we can do
find test cases which exercise bugs. Hopefully the current tests are
sufficient for the use cases you are looking at.

Thank you.

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

signature.asc
Description: This is a digitally signed message part