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

feat(execution): implement more environments logic #64

Merged
merged 11 commits into from
Nov 24, 2023
Merged

feat(execution): implement more environments logic #64

merged 11 commits into from
Nov 24, 2023

Conversation

sno2
Copy link
Contributor

@sno2 sno2 commented Nov 3, 2023

This will place us in a better position for a basic bytecode later.

@sno2 sno2 marked this pull request as draft November 3, 2023 01:25
ghost
ghost reviewed Nov 3, 2023
View reviewed changes
Copy link

@ghost ghost left a comment •
edited by ghost
Loading

Choose a reason for hiding this comment

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

This looks well! work should come up soon on the interpreter, approx next week, so this is great to have in advance :)

nova_vm/src/ecmascript/execution/environments.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/declarative_environment.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/function_environment.rs Outdated Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments.rs Outdated Show resolved Hide resolved
aapoalas
aapoalas previously approved these changes Nov 3, 2023
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Looks good to me.

nova_vm/src/ecmascript/execution/environments/declarative_environment.rs Outdated Show resolved Hide resolved
@ghost
Copy link

ghost commented Nov 3, 2023 •
edited by ghost
Loading

Looks good to me.

It's a draft, lol.

But I get the idea that we're both in approval of the draft

@sno2 sno2 marked this pull request as ready for review November 4, 2023 19:43
@sno2 sno2 changed the title feat(execution): environments work feat(execution): implement more of environments Nov 4, 2023
@sno2 sno2 changed the title feat(execution): implement more of environments feat(execution): implement environments logic Nov 4, 2023
@sno2 sno2 changed the title feat(execution): implement environments logic feat(execution): implement more environments logic Nov 4, 2023
@sno2 sno2 mentioned this pull request Nov 7, 2023
Merged
@sno2 sno2 requested review from a user and aapoalas November 7, 2023 16:55
aapoalas
aapoalas approved these changes Nov 7, 2023
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Had some thoughts and maybe some nitpicks but nothing really worth thinking about too deeply.

nova_vm/src/ecmascript/execution/environments.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/declarative_environment.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/declarative_environment.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/declarative_environment.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/global_environment.rs Show resolved Hide resolved
nova_vm/src/ecmascript/execution/environments/global_environment.rs Show resolved Hide resolved
@sno2 sno2 enabled auto-merge (squash) November 8, 2023 13:57
@aapoalas aapoalas enabled auto-merge (squash) November 24, 2023 18:10
@aapoalas aapoalas merged commit 2e55607 into main Nov 24, 2023
1 check passed
@aapoalas aapoalas deleted the feat/environments branch November 24, 2023 18:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapoalas aapoalas aapoalas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sno2 @aapoalas