Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Bug - escapeFromConditionalBranches #472

Merged
merged 3 commits into from
Mar 1, 2017
Merged

Bug - escapeFromConditionalBranches #472

merged 3 commits into from
Mar 1, 2017

Conversation

griffpatch
Copy link
Contributor

Fixes Bug: Execution bug: can't escape from conditional branches that end with a Promise-resolution-terminating command block #464

@griffpatch
Merge remote-tracking branch 'refs/remotes/LLK/develop' into bug/esca…
682dec4 
…peFromConditionalBranches
@griffpatch
bug - escape from conditional branches
362a231 
Execution bug: can't escape from conditional branches that end with a
Promise-resolution-terminating command block (see scratchfoundation#464)
@griffpatch
Copy link
Contributor Author

Wait... hold off on that... this is not a good fix as I forgot to account for promises at the end of actual loops. This will break that scenario at present.

@griffpatch
Handle actual looping cases
c5eb8ec 
And it starts to get a little less elegant :/
Wondering if this should not be handled better in another part of the
codebase?
We don't want to be duplicating existing code stepping functionality
locally at the end of the promise script really... What do you think?
@thisandagain thisandagain self-requested a review February 20, 2017 22:49
@thisandagain thisandagain self-assigned this Feb 20, 2017
@thisandagain thisandagain added this to the March 23 milestone Feb 20, 2017
@griffpatch
Copy link
Contributor Author

Forgot to say, this fix is ok now and can be looked at (if it helps).
My only hesitation is that it would be nicer if the scripts to exit the current stack frame did not have to live in this bit of the codebase as this is usually done in the stepThread function rather than the execute function... but since the popThread and getNextBlockID was already being done in the later for the isPromise callback I added the other logic to look for end of execution and loop detection in there too to fix the bug.

thisandagain
thisandagain approved these changes Mar 1, 2017
View reviewed changes
Copy link
Contributor

@thisandagain thisandagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @griffpatch . This looks good. Sorry for the slow review on this, but I just wanted to make sure to test it thoroughly.

@thisandagain thisandagain merged commit 7042bdb into scratchfoundation:develop Mar 1, 2017
@griffpatch
Copy link
Contributor Author

griffpatch commented Mar 1, 2017 via email

@griffpatch griffpatch deleted the bug/escapeFromConditionalBranches branch March 28, 2017 08:58
@griffpatch griffpatch mentioned this pull request Mar 28, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thisandagain thisandagain thisandagain approved these changes

Assignees

@thisandagain thisandagain

Labels
pr - good to merge
Projects
None yet
Milestone
March 23
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@griffpatch @thisandagain