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

[Matt Lilley]

On 2/19/2012 6:40 AM, Daniel Thomas wrote:
> Sorry for the months of delay. This has now been pushed with only minor
> changes.
Thanks for your time in considering my proposal!
> 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.
I can certainly live with that. The proposal was based on SWI-Prolog's 
implementation, and I think from reading the documentation that they do 
this as well, though it is marked as a bug. In other words, we sort of 
already expect this behaviour.
> The code also got reformatted as I have eclipse set to vigorously
> enforce GNU's code formatting guidelines.
Sure. I tried to follow the layout from the rest of the project, but I 
may have slipped up once or twice. Strange that GNU emacs doesn't do as 
good a job as eclipse!
> I moved the unit test you added into extra as it is not testing a core
> feature.
That's fine.
> 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)?
If it's not too much effort, a 0.2.x branch would be much appreciated.

Thanks again,
Matt
> 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


--
_____________________________________________
Matt Lilley
Software Engineer
SecuritEase

Tel:    +64 4 912-2100
Fax:    +64 4 912-2101
E-mail: address@hidden
Web:    http://www.securitease.com
_____________________________________________

This e-mail has passed our content security scan.
It is covered by the confidentiality clauses at 
http://www.securitease.com/content_and_confidentiality