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

[Daniel Thomas]

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