-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[STOP] quit the superprocedure when inside [REPEAT] #137
Comments
Yup, it fails for me too. Making it [print "Right! stop foo] (so that the STOP isn't a tail call) doesn't change the result (still fails). Making it REPEAT 2 instead of 1 (so the user's correct answer isn't in the final repetition) also still fails. Putting PR "FOO after the two test.once lines in test.twice (so test.once isn't a tail call) still fails. Putting a print between the two test.once calls also doesn't change anything. On the other hand, eliminating the REPEAT, so that the STOP gets directly to the procedure call frame, does fix the problem. It's only if stopping_flag==STOP Changing the REPEAT 1 to a RUN makes it work correctly, so there's something specifically wrong with REPEAT. I swear all the CSLS code used to work... :( |
Appericated for the prompt reply, especially from the author of the book :) I had also done a few experiments before asking here. Besides eliminating REPEAT like you mentioned, changing STOP to OUTPUT "(and adding IGNORE in test.twice) fixes the problem as well. I also defined a test.quad that calls test.twice twice, the 2nd test.twice does get excuted, just with the same bug. As long as I know it's not my misunderstanding of STOP I can look over it and continue the book :) BTW, I really like your CSLS, wish I had read it much earlier. |
Thanks! Your test.quad experiment will be helpful. It tells us that the STOP isn't unwinding the stack all the way to toplevel, just that it unwinds once too often. I hope someone else debugs this; even though I wrote the evaluator, it terrifies me every time I have to look at it. :( |
I'll gladly take a look at any pull requests to fix this, otherwise it will probably be October before I have time to take a look. |
Note to future self: Figure out what is happening at eval_sequence_continue in eval.c as test.twice is run. |
Hm, yes, the evaluator is somewhat terrifying. @brianharvey Can you suggest some sample code I can use for the testing this? I would like to avoid breaking it in a new way. I did check, and Ucblogo 6.1 has the same problem, so it is at least three years old. |
So if I did my logo code correctly, both of these should output "true but because of this bug, outer2 outputs "false:
|
Two more examples, inner3 is broken, inner4 works:
|
FYI, if in #139, I uncomment Tests.Macro.RepeatEarlyStopWorksAsExpected I get this error:
Update: Just a weird error message if the name of the test is too long. Nevermind. |
Debug prints from outer2:
|
Debug prints from outer1:
|
I am running this program to test it (in the debug_stop branch):
and basically, I am seeing this:
The relevant code is: eval_sequence_continue:
reset_args(var);
no_reset_args: /* allows catch "foo [local ...] to work */
eval_restore();
if (dont_fix_ift) {
ift_iff_flag = dont_fix_ift-1;
dont_fix_ift = 0;
}
debprint("eval_sequence_continue");
if (stopping_flag == MACRO_RETURN) {
if (val != NIL && is_list(val) && (isName(car(val), Name_tag)))
unev = cdr(val); /* from goto */
else
unev = append(val, unev);
val = UNBOUND;
stopping_flag = RUN;
if (unev == NIL) goto fetch_cont;
} else {
if (current_unode != output_unode) {
if (STOPPING || RUNNING) output_node = UNBOUND;
if (stopping_flag == OUTPUT || STOPPING) {
stopping_flag = RUN;
val = output_node;
goto fetch_cont;
}
}
} Essentially, eval_restore is getting called twice, and wiping out the middle function's remaining code. |
Hm, with 5ecb963 it works:
I do worry that there is some corner case that needs |
Besides the ones I have already mentioned, here are the test cases I have used: Should return "d
Should return "c
Should return "b
|
Ah, here is a test that fails with my "fix":
|
I have fixed the fix, but I still am not sure it is fixed: 2e15631 If anyone has suggestions for more logo code to test with, that would be good. |
One hairy case that might go wrong involves FOR: to test |
Yes, this works, and I added it to the tests in the branch :) |
I was reading through CSLS and encountered this weird problem
when I run test.twice in ucblogo 6.2(windows) I got
It seems that the [STOP] in test.once also quits test.twice and returns to toplevel. Could you please take a look into this? Thanks!
The text was updated successfully, but these errors were encountered: