Skip to content

Conversation

@mzgoddard
Copy link
Contributor

Resolves

Related to #1729

Proposed Changes

Initialize parameters before pushing parameters onto a procedure's stack frame. This way all procedure stack frames define params. (Not all stack frames are for procedures. Some are for branching blocks like if or forever.)

Reason for Changes

A previous change fixed compatibility with Scratch 2, removing 3's unintentional scope leaking. This furthers that change so that procedures with no parameters will also not accidentally use values in other procedure stacks.

Test Coverage

Add an additional execute integration test following the existing procedures-nested-missing-number-param.sb2 test.

@mzgoddard
Copy link
Contributor Author

  • Add some comments to the new methods

A previous change fixed compatibility with Scratch 2 removing 3's
unintentional scope leaking. This furthers that change so that
procedures with no parameters will also not accidentally use values in
other procedure stacks.
@mzgoddard mzgoddard force-pushed the fix-missing-no-param branch from c812fce to 20ff75b Compare December 13, 2018 15:51
@mzgoddard
Copy link
Contributor Author

  • Fix unit thread unit test that should call the new initParams function.

Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

LG but requesting documentation =]

Co-Authored-By: mzgoddard <mzgoddard@gmail.com>
Copy link
Contributor

@kchadha kchadha left a comment

Choose a reason for hiding this comment

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

LGTM

@kchadha
Copy link
Contributor

kchadha commented Jan 15, 2019

Including this screen shot in here as a good test case:

image

This comes from the project mentioned in #1729

@kchadha kchadha self-assigned this Jan 15, 2019
@kchadha kchadha merged commit 0fa5e91 into scratchfoundation:develop Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants