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

feat(hog): bytecode versions #24584

Merged
merged 29 commits into from
Aug 27, 2024
Merged

feat(hog): bytecode versions #24584

merged 29 commits into from
Aug 27, 2024

Conversation

mariusandra
Copy link
Collaborator

Problem

Continuing extraction from #24369

Changes

  • Add a version field to Hog bytecode.
  • Introduce version 1, which turns around the order of arguments on the stack for function calls. This makes it possible to support optional arguments in the future.
  • Rename op.CALL to op.CALL_GLOBAL
  • Throw if trying to access a global variable that's not found
  • Small cleanup here and there

How did you test this code?

Updated many tests

@mariusandra mariusandra requested a review from a team August 26, 2024 20:02
Copy link
Contributor

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Mostly looked at tests. I'm assuming then at some point we will need a migration layer to ensure backwards compat? I couldn't quite parse in all the code if there is a chance other than supporting a version index?

@mariusandra
Copy link
Collaborator Author

Other than the version integer in the bytecode, the code also 1) flips around the arguments in function calls when using the new version, 2) changes CALL to CALL_GLOBAL, 3) throws on undefined global access

All of it is effectively housekeeping in order to make the next extracted PR (actually lambdas and function refactors) easier to swallow.

We won't need a migration as long as we keep the if version != 0 check in one place in the VMs. I explicitly built it this way so that we wouldn't need a breaking "rebuild all" migration for Hog functions.

Base automatically changed from lambda-parser to master August 27, 2024 09:21
An error occurred while trying to automatically change base from lambda-parser to master August 27, 2024 09:21
@mariusandra mariusandra merged commit b7105b6 into master Aug 27, 2024
86 checks passed
@mariusandra mariusandra deleted the bytecode-versions branch August 27, 2024 15:04
mariusandra added a commit that referenced this pull request Aug 27, 2024
pauldambra pushed a commit that referenced this pull request Aug 29, 2024
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.

2 participants