-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
VM: Implement variable declaration (var, const, and let) #1030
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1030 +/- ##
==========================================
- Coverage 58.72% 58.39% -0.33%
==========================================
Files 172 172
Lines 11728 11861 +133
==========================================
+ Hits 6887 6926 +39
- Misses 4841 4935 +94
Continue to review full report at Codecov.
|
I made this support |
let
keyword
boa/src/vm/instructions.rs
Outdated
|
||
DefVar(usize, usize), | ||
DefLet(usize, usize), | ||
DefConst(usize, usize), |
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 add some documentation/comments to these? So others know what the 2 usize arguments are for
As a bonus putting these in some alphabetical order would be nice (but not essential)
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.
We could even use attributes in a struct-like variant, with names. Would make things much more clear.
- DefLet doesn't need the value - InitLexical initializes value if set
let a = 1; generates
@AnnikaCodes if you look at the changes ive made, letDecl now doesn't use run instead it calls There's optimization that can be done here, we know DefLet will always be a string, so we don't need to convert it back and forth between a Value and a String. It will be converted from a |
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.
From my side, I think a bit of documentation is needed in the definition of the enums. Give it a look, but from the rest, I'm happy with it :)
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.
Yep looks good to me.
Thanks for your contribution AnnikaCodes !
This Pull Request fixes #1028.
It changes the following:
DefVar
VM instructionContext
in the function signature ofCodeGen::compile
LetDeclList
s intoDefVar
instructions