-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
Switch impl #451
Switch impl #451
Conversation
Co-authored-by: Iban Eguia <razican@protonmail.ch>
Switch part divided up from run
Divided up GetConstField, GetField
Divide run dowhile
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for #422 to land to easily see the changes here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking very good. Check my comments, I think we can improve this to avoid having tens of flags in the future :)
boa/src/exec/break_node/mod.rs
Outdated
|
||
impl Executable for Break { | ||
fn run(&self, interpreter: &mut Interpreter) -> ResultValue { | ||
interpreter.is_break = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, we could have a break(label: &str)
function (signature might differ) in interpreter
, which would change the state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we also add tests for breaks in loop statements?
Done, I also got rid of the .is_return() etc. in favour of just matching on get_current_state() based on the assumption that almost all the time in the code if .is_return() needs to be handled then so does .is_break() so might as-well make this required to prevent accidental omissions. I haven't attempted to actually add any handling of break labels https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/label (this might be better as a separate issue?) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me, thanks!
This Pull Request implements #439 and #301.
It changes the following:
A function parse_generalised in statement list was created as part of this and is a more generalised version of StatementList::parse() which allows statement lists to be terminated by tokens other than '}'. It's a separate function for now so that the performance impact of replacing parse can be established.
This pull request is based of PR #422.