-
Notifications
You must be signed in to change notification settings - Fork 683
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
Conversation
@@ -0,0 +1,20 @@ | |||
// Copyright 2015 Samsung Electronics Co., Ltd. | |||
// |
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.
Add missing (c), please.
Good to me. |
28d0d69
to
79fed67
Compare
@egavrin, I've updated pull request according to your comments:
|
} | ||
} | ||
} | ||
assert (i === 10); |
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.
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");
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.
Thanks for suggestion. I'll update the test.
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. |
79fed67
to
ac17aad
Compare
@zherczeg, the pull request is fixed according to your suggestion. |
Blocked by #147 |
thank you. I have nothing more to add. |
ac17aad
to
77af5eb
Compare
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)); |
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.
Instead the assert should we just check if the with_completion
is an empty value in the if
check?
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.
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;
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, sure. I've updated pull request.
21350ac
to
b2174e1
Compare
@galpeter, I've updated pull request according to your review comments. |
@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, 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
b2174e1
to
ac87616
Compare
Related issues: #52, #128.