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
0001-Fixed-Do-not-call-cleanup-when-backtracking-in-to-th.patch