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.

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

don't use branchCount for extension substack compilation #182

Closed
wants to merge 1 commit into from

Conversation

CST1229
Copy link

@CST1229 CST1229 commented Jan 13, 2024

Resolves

Resolves 🗨︎(extremely minor parity issue) Compiler uses branchCount for startBranch

Proposed Changes

Makes the compiler use all inputs starting with SUBSTACK for extension C block compilation rather than relying on the block info's branchCount.

Reason for Changes

This fixes a minor parity issue with the interpreter where if the number of SUBSTACK inputs in a block was more than its branchCount (e.g for blocks with mutators), the extra substacks didn't compile.

Test Coverage

Haven't touched tests.

This fixes a minor parity issue with the interpreter where if the number of SUBSTACK inputs in a block was more than its branchCount (e.g for blocks with mutators), the extra substacks didn't compile.
@GarboMuffin
Copy link
Member

just like last time github won't let me push to this pull request

i cherry-picked this into develop, hopefully that still gives you commit credit?

@CubesterYT
Copy link
Member

CubesterYT commented Jan 14, 2024

Maybe it doesn't let merging because CSTs fork is a fork of the original scratch VM?

@GarboMuffin
Copy link
Member

in hindsight i would've been able to just press the merge button instead of cherry-picking since all i had to do was write tests

@CST1229
Copy link
Author

CST1229 commented Jan 14, 2024

just like last time github won't let me push to this pull request

actually it may be related to weird git stuff (because i'm making a branch based off of turbowarp on a fork based on vanilla)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants