-
Notifications
You must be signed in to change notification settings - Fork 676
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
Fix parse of simple for statement #237
Conversation
Good to me. |
lexer_seek (for_open_paren_loc); | ||
tok = lexer_next_token (); | ||
|
||
bool is_plain_for = jsp_find_next_token_before_the_locus (TOK_SEMICOLON, |
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.
Maybe, blocks, surrounded with braces, should be skipped during the search.
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.
Fixed.
Is there a test case for checking function call in init and condition part? ex: for (var i = init(); i < max(); i = dec(i)) {} Could you add something similar to |
907eec1
to
2bcec7d
Compare
@LaszloLango, I've added tests to |
The following JS codes are now failing with this PR (note the empty lines!): for (
var i = {x: 0};
i.x < 2
;
i.x++
)
{
print (i.x);
} Output:
and for the: for (
var i = {x: 0};
i.x < 2
;
i.x++
)
{
print (i.x);
} Output:
We should also add these as test cases. |
@galpeter, thank you for the test cases. I'll check them and update pull request. |
* Missing corresponding brace is considered a syntax error | ||
* | ||
* Note: | ||
* current token should at the beginning of the scope when the routine is called |
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 you please give a bit more detail on what to you mean by this?
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.
Yes, I'll update the comment.
It is currently incorrect: "should at" -> "should be at", "scope" -> "block".
The comment means that token, set as "current" in parser context, should be at the beginning of the block to skip. In other words, current position should be set a beginning of the block to skip.
Is the following version ok: "Opening brace of the block to skip should be set as current token when the routine is called"?
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.
Yes, that would be okay.
2bcec7d
to
2058c26
Compare
@galpeter, I've updated pull request:
Please, review. |
|
||
) | ||
{ | ||
print (i.x); |
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 should not have prints in the tests, maybe we could replace it with some numeric operation and check after the for
if the calculation was ok.
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.
Ok.
2058c26
to
d9121f4
Compare
@galpeter, I've updated pull request:
|
lgtm |
|
…registered during main parse stage. JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan [email protected]
…h braces. JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan [email protected]
Related issue: #156 JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan [email protected]
d9121f4
to
f849cc6
Compare
Related issue: #156