label statement implementation #549
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Benchmark for b88e16aClick to view benchmark
|
Codecov Report
@@ Coverage Diff @@
## master #549 +/- ##
==========================================
- Coverage 71.78% 71.74% -0.04%
==========================================
Files 199 200 +1
Lines 13943 13980 +37
==========================================
+ Hits 10009 10030 +21
- Misses 3934 3950 +16
Continue to review full report at Codecov.
|
Benchmark for fc915c7Click to view benchmark
|
I think we should have a
I think it should be the |
Razican
reviewed
Jul 6, 2020
| /// [mdn]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/label | ||
| /// [spec]: https://tc39.es/ecma262/#sec-labelled-statements | ||
| #[derive(Debug, Clone, Copy)] | ||
| pub(super) struct Label { |
There was a problem hiding this comment.
I would keep the name of the LabelledStatement as in the spec. This parser would return a LabelledStatement node, with a label and a Node.
Closed
Benchmark for c3c395cClick to view benchmark
|
|
@Razican i can't think of how we would do this without either passing a label into the run method, but that would mean changing the interface for all Execution, or passing the label when However we do this we need to access the label here |
|
I've had another look at this, and i think we could have a stack for labels on the Then the break operation can check against the top label on the stack. Edit: Edit: |
|
A much simpler implementation would be to add the label to the loop statement AST nodes. Then, the loops can simply: InterpreterState::Break(label) => {
// Loops 'consume' breaks, only if the label is not present, or does not match the current loop.
match label {
None => interpreter.set_current_state(InterpreterState::Executing),
Some(_) if label == self.label() => interpreter.set_current_state(InterpreterState::Executing),
_ => {}
}
break;
}
// ... Likewise for Continue.I'm pretty certain that would work. This uses the native/Rust stack as the label stack, completely avoiding that allocation. Obviously, this allows invalid JS: label1: while(true) {
break label2;
}So the validity of labels would need to be checked in the parser. |
|
The issue with this approach is that it only allows labelled loops, but, functions and other statements can also have a label: https://tc39.es/ecma262/#sec-labelled-statements |
|
The direction I’ve gone in is that Nodes which currently use a label will have a label property and use it as needed. So not too different from your example. If we can set the label on the statement then there’s no need for a label node during execution, also if we need to add labels to new nodes in future it will be easy. If a Node doesn’t need a label it’s basically a noop and that statement is just returned “as-is”. This is achieved by matching on a node and calling set_label on the ones that can. It can’t be set at execution time because the Node is in a “read-only” state at this point (we can only run it) |
Razican
reviewed
Sep 2, 2020
There was a problem hiding this comment.
After the last comment, I thought we would have stuff such as an Option<Box<str>> for a label in some nodes, but the code here still has a Label node. I did a review with some comments though.
I do like the idea of having an optional label in the statements that allow it, that would make everything easier, and I don't think it will grow memory usage too much.
jasonwilliams
force-pushed
the
feature/548
branch
from
8825e4b to
fb11b15
Compare
|
Label Node is no longer needed. Then execution steps remains as it was before (no change there).
|
Benchmark for 46c28baClick to view benchmark
|
Benchmark for 23e37bdClick to view benchmark
|
jasonwilliams
force-pushed
the
feature/548
branch
from
9733f78 to
9a98fab
Compare
jasonwilliams
commented
Sep 21, 2020
Benchmark for 6b1a49fClick to view benchmark
|
Benchmark for dd4fe8cClick to view benchmark
|
jasonwilliams
force-pushed
the
feature/548
branch
from
1b70069 to
42fd538
Compare
Benchmark for 3e15427Click to view benchmark
|
Benchmark for 1bf525aClick to view benchmark
|
HalidOdat
added
enhancement
Benchmark for 9a6d698Click to view benchmark
|
Benchmark for 731adacClick to view benchmark
|
All exec needs to do now is run the statement. `Label` node is still needed because we don't know what the statement is
Instead the Label parser modifies the Node to have a label then returns that Node.
jasonwilliams
force-pushed
the
feature/548
branch
from
fd63c1e to
e723117
Compare
HalidOdat
reviewed
Sep 27, 2020
Co-authored-by: Halid Odat <halidodat@gmail.com>
Benchmark for cf1da03Click to view benchmark
|
Benchmark for cf1da03Click to view benchmark
|
HalidOdat
approved these changes
Sep 28, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #549
@Razican it looks like the Statement parse will need to be passed an optional
labelargument which then is also passed into all of the sub statements, what do you think?Also what is the Output type for labelled Statement? As it technically, it is allowed to wrap any statementI think this could be NodeTry with