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

[Daniel Thomas]

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