Skip to content

Landing patch for labelled statements #142

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

Merged
merged 3 commits into from
Jun 4, 2015

Conversation

ruben-ayrapetyan
Copy link
Contributor

Related issues: #52, #128.

@ruben-ayrapetyan ruben-ayrapetyan added bug Undesired behaviour normal parser Related to the JavaScript parser interpreter Related to the virtual machine ecma core Related to core ECMA functionality development Feature implementation labels Jun 2, 2015
@ruben-ayrapetyan ruben-ayrapetyan added this to the Core ECMA features milestone Jun 2, 2015
@egavrin egavrin changed the title Preparations for implementation of labelled statements Landing patch for labelled statements Jun 2, 2015
@@ -0,0 +1,20 @@
// Copyright 2015 Samsung Electronics Co., Ltd.
//
Copy link
Contributor

Choose a reason for hiding this comment

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

Add missing (c), please.

@egavrin
Copy link
Contributor

egavrin commented Jun 2, 2015

Good to me.
@galpeter please review.

@egavrin egavrin assigned galpeter and unassigned egavrin Jun 2, 2015
@ruben-ayrapetyan ruben-ayrapetyan force-pushed the Ruben-fix-simple-breaks branch 2 times, most recently from 28d0d69 to 79fed67 Compare June 2, 2015 17:16
@ruben-ayrapetyan
Copy link
Contributor Author

@egavrin, I've updated pull request according to your comments:

  • added missing copyright notice;
  • renamed 'break_continue' completion value type to 'jump'.

}
}
}
assert (i === 10);
Copy link
Member

Choose a reason for hiding this comment

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

I think that such assert does not prove whether this works correctly. I would recommend a string concatenation:

var str = "";
for (var i = 0; i < 10; i++)
{
  with ({})
  {
    switch (i)
    {
      case 0:
        str += "A";
        break;
      default:
        str += "B";
        continue;
    }
    str += "C";
  }
}
assert(str === "ACBBBBBBBBB");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion. I'll update the test.

@zherczeg
Copy link
Member

zherczeg commented Jun 3, 2015

A possible optimization: when the ecma return stuff will be thrown out later, these statements could be turned to simple jumps. Much faster, much less code.

@ruben-ayrapetyan
Copy link
Contributor Author

@zherczeg, the pull request is fixed according to your suggestion.

@ruben-ayrapetyan
Copy link
Contributor Author

Blocked by #147

@zherczeg
Copy link
Member

zherczeg commented Jun 3, 2015

thank you. I have nothing more to add.

@ruben-ayrapetyan ruben-ayrapetyan force-pushed the Ruben-fix-simple-breaks branch from ac17aad to 77af5eb Compare June 3, 2015 13:58
JERRY_ASSERT (meta_opcode.data.meta.type == OPCODE_META_TYPE_END_WITH);
if (ecma_is_completion_value_normal (with_completion))
{
JERRY_ASSERT (ecma_is_completion_value_empty (with_completion));
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead the assert should we just check if the with_completion is an empty value in the if check?

Copy link
Contributor

Choose a reason for hiding this comment

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

So something like this maybe:

if (ecma_is_completion_value_empty (with_completion))
{
  JERRY_ASSERT(..);
  int_data->pos++;
}
else
{
   JERRY_ASSERT(..);
}

ret_value = with_completion;

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, sure. I've updated pull request.

@ruben-ayrapetyan ruben-ayrapetyan force-pushed the Ruben-fix-simple-breaks branch 3 times, most recently from 21350ac to b2174e1 Compare June 4, 2015 10:52
@ruben-ayrapetyan
Copy link
Contributor Author

@galpeter, I've updated pull request according to your review comments.

@galpeter
Copy link
Contributor

galpeter commented Jun 4, 2015

@ruben-ayrapetyan, looks good to me. Just one question: it seems that the automated system couldn't execute the build/test correct. Do you know what could be the problem?

@ruben-ayrapetyan
Copy link
Contributor Author

@galpeter, the problem is caused by incorrect parsing of default case in switch-case statement (should be fixed by #147 - this is why current pull request is blocked by #147).

@galpeter
Copy link
Contributor

galpeter commented Jun 4, 2015

@ruben-ayrapetyan, ah okay, makes sense.

JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
Related issue: #128

JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Undesired behaviour development Feature implementation ecma core Related to core ECMA functionality interpreter Related to the virtual machine normal parser Related to the JavaScript parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants