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

[Merged by Bors] - Labelled ByteCompiler Fix #2534

Closed
wants to merge 4 commits into from

Conversation

nekevss
Copy link
Member

@nekevss nekevss commented Jan 15, 2023

This Pull Request addresses #2295, and another case that I came across when I was adding Break to the ByteCompiler

I did have a question that came up during this regarding the spec. We currently don't implement the BreakableStatement. Any thoughts on whether we should be? Especially since BreakableStatement seems to be a bit of a inaccurate since LabelledStatement is breakable too.

It changes the following:

  • Moves handling of label jump out of compile_block and into compile_labelled.
  • Adds a couple more tests to keep track of LabelledStatement breaks.

@nekevss nekevss added execution Issues or PRs related to code execution vm Issues and PRs related to the Boa Virtual Machine. labels Jan 15, 2023
@nekevss nekevss added this to the v0.17.0 milestone Jan 15, 2023
@codecov
Copy link

codecov bot commented Jan 15, 2023

Codecov Report

Merging #2534 (0335e1e) into main (20af6a7) will increase coverage by 0.18%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2534      +/-   ##
==========================================
+ Coverage   49.92%   50.11%   +0.18%     
==========================================
  Files         377      377              
  Lines       37557    37438     -119     
==========================================
+ Hits        18751    18761      +10     
+ Misses      18806    18677     -129     
Impacted Files Coverage Δ
boa_engine/src/bytecompiler/jump_control.rs 85.71% <100.00%> (-0.09%) ⬇️
boa_engine/src/bytecompiler/statement/block.rs 100.00% <100.00%> (ø)
boa_engine/src/bytecompiler/statement/labelled.rs 46.87% <100.00%> (-1.70%) ⬇️
boa_engine/src/bytecompiler/statement/mod.rs 97.05% <100.00%> (ø)
boa_cli/src/main.rs 0.00% <0.00%> (-1.08%) ⬇️
boa_engine/src/vm/mod.rs 48.75% <0.00%> (-1.00%) ⬇️
boa_ast/src/punctuator.rs 44.14% <0.00%> (-0.91%) ⬇️
boa_gc/src/cell.rs 58.75% <0.00%> (-0.57%) ⬇️
boa_engine/src/object/mod.rs 45.37% <0.00%> (-0.11%) ⬇️
boa_cli/src/helper.rs 0.00% <0.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@nekevss nekevss force-pushed the labelled-compiler-fix branch from c776af7 to 0335e1e Compare January 18, 2023 14:25
@nekevss nekevss removed the execution Issues or PRs related to code execution label Jan 18, 2023
@raskad
Copy link
Member

raskad commented Jan 19, 2023

I did have a question that came up during this regarding the spec. We currently don't implement the BreakableStatement. Any thoughts on whether we should be? Especially since BreakableStatement seems to be a bit of a inaccurate since LabelledStatement is breakable too.

Since there are no early errors or specific places where execution depends on the statements being BreakableStatement, I think it is fine to not implement it directly.

Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Nice change and godd to add some more tests for break!

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Nice improvements!

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Jan 19, 2023
<!---
Thank you for contributing to Boa! Please fill out the template below, and remove or add any
information as you feel necessary.
--->

This Pull Request addresses #2295, and another case that I came across when I was adding `Break` to the `ByteCompiler`

I did have a question that came up during this regarding the spec. We currently don't implement the [BreakableStatement](https://tc39.es/ecma262/#prod-BreakableStatement). Any thoughts on whether we should be? Especially since `BreakableStatement` seems to be a bit of a inaccurate since `LabelledStatement` is breakable too.

It changes the following:

- Moves handling of label jump out of `compile_block` and into `compile_labelled`.
- Adds a couple more tests to keep track of `LabelledStatement` breaks.


Co-authored-by: Ness <[email protected]>
@bors
Copy link

bors bot commented Jan 19, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Labelled ByteCompiler Fix [Merged by Bors] - Labelled ByteCompiler Fix Jan 19, 2023
@bors bors bot closed this Jan 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the Boa Virtual Machine.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants