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

Fix parse of simple for statement #237

Merged
merged 3 commits into from
Jun 25, 2015
Merged

Conversation

ruben-ayrapetyan
Copy link
Contributor

Related issue: #156

@ruben-ayrapetyan ruben-ayrapetyan added bug Undesired behaviour normal parser Related to the JavaScript parser labels Jun 24, 2015
@ruben-ayrapetyan ruben-ayrapetyan added this to the Core ECMA features milestone Jun 24, 2015
@egavrin
Copy link
Contributor

egavrin commented Jun 24, 2015

Good to me.

@egavrin egavrin assigned galpeter and unassigned egavrin Jun 24, 2015
lexer_seek (for_open_paren_loc);
tok = lexer_next_token ();

bool is_plain_for = jsp_find_next_token_before_the_locus (TOK_SEMICOLON,
Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@LaszloLango
Copy link
Contributor

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 tests/jerry/for.js too?

@ruben-ayrapetyan
Copy link
Contributor Author

@LaszloLango, I've added tests to tests/jerry/regression-test-issue-156.js.

@galpeter
Copy link
Contributor

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:

 i.x < 2
^
ERROR: Ln 4, Col 1: Unknown token newline

and for the:

for (
var i = {x: 0};

 i.x < 2
;

 i.x++

)
 {
  print (i.x);
}

Output:

 i.x++
^
ERROR: Ln 7, Col 1: Unknown token newline

We should also add these as test cases.

@ruben-ayrapetyan
Copy link
Contributor Author

@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
Copy link
Contributor

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?

Copy link
Contributor Author

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"?

Copy link
Contributor

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.

@ruben-ayrapetyan
Copy link
Contributor Author

@galpeter, I've updated pull request:

  • fixed newline handling in 'condition' and 'increment' parts (the change fixes test cases, provided by you);
  • added the test cases to 'tests/jerry/for.js'.

Please, review.


)
{
print (i.x);
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

@ruben-ayrapetyan
Copy link
Contributor Author

@galpeter, I've updated pull request:

  • removed 'print' calls from tests, and added assertion checks for the values.

@galpeter
Copy link
Contributor

lgtm

@galpeter galpeter removed their assignment Jun 25, 2015
@egavrin
Copy link
Contributor

egavrin commented Jun 25, 2015

make push

…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]
@ruben-ayrapetyan ruben-ayrapetyan merged commit f849cc6 into master Jun 25, 2015
@egavrin egavrin deleted the Ruben-fix-issue-156 branch June 30, 2015 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Undesired behaviour normal parser Related to the JavaScript parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants