Skip to content

Implemention of eval (by Andrey Shitov and Ruben Ayrapetyan) #174

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Jun 13, 2015
Merged

Implemention of eval (by Andrey Shitov and Ruben Ayrapetyan) #174

merged 8 commits into from
Jun 13, 2015

Conversation

ruben-ayrapetyan
Copy link
Contributor

Related issue: #48

@ruben-ayrapetyan ruben-ayrapetyan added normal parser Related to the JavaScript parser ecma core Related to core ECMA functionality ecma builtins Related to ECMA built-in routines development Feature implementation labels Jun 11, 2015
@ruben-ayrapetyan ruben-ayrapetyan added this to the Core ECMA features milestone Jun 11, 2015
@ruben-ayrapetyan ruben-ayrapetyan force-pushed the eval_final_dev branch 2 times, most recently from 101b78b to 7c49630 Compare June 11, 2015 14:15
galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
tests/jerry/eval.js
assert (typeof (y) === 'string');

delete y;
assert (typeof (y) === 'string');
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be "undefined" instead of "string"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, as y is variable, declared through 'var' in global code (10.4.1), and so configurableBindings for the variable is false (10.5).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah.. okay I see.

@ruben-ayrapetyan
Copy link
Contributor Author

Pull request is updated: added two more test cases for 'eval'.

@ruben-ayrapetyan
Copy link
Contributor Author

Please, review.

galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
jerry-core/parser/js/parser.cpp
if (this_arg_p != NULL
&& !operand_is_empty (*this_arg_p))
{
call_flags = (opcode_call_flags_t) (call_flags | OPCODE_CALL_FLAGS_HAVE_THIS_ARG);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think call_flags |= OPCODE_CALL_FLAGS_HAVE_THIS_ARG would be better

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, it seems to be more readable, but current compiler options do not permit the version:
error: invalid conversion from ‘int’ to ‘opcode_call_flags_t’ [-fpermissive].

@egavrin
Copy link
Contributor

egavrin commented Jun 11, 2015

Strange name of pull request. I feel myself excluded ^_^

egavrin
egavrin reviewed Jun 11, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-function.cpp
{
is_strict = true;
}
if ((scope_flags & OPCODE_SCOPE_CODE_FLAGS_NOT_REF_ARGUMENTS_IDENTIFIER)
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing empty line.

@ruben-ayrapetyan
Copy link
Contributor Author

@egavrin, I've updated pull request according to your note.

galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
jerry-core/vm/opcodes-ecma-try-catch-finally.cpp
JERRY_ASSERT (next_opcode.op_idx == __op__idx_meta);
JERRY_ASSERT (next_opcode.data.meta.type == OPCODE_META_TYPE_CATCH_EXCEPTION_IDENTIFIER);

lit_cpointer_t catch_exc_val_var_name_lit_cp = serializer_get_literal_cp_by_uid (
next_opcode.data.meta.data_1,
int_data->pos);
int_data->opcodes_p, int_data->pos);
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO: the last argument should be on a new line if the other parameters are already separated by a newline character.

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, I'll update.

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

galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
jerry-core/vm/vm.cpp

#ifdef MEM_STATS
const opcode_counter_t opcode_pos = int_data_p->pos;

interp_mem_stats_opcode_enter (opcode_pos,
interp_mem_stats_opcode_enter (int_data_p->opcodes_p, opcode_pos,
Copy link
Contributor

Choose a reason for hiding this comment

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

Parameters should each be on it's own line in this case.

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

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

@sand1k
Copy link
Contributor

sand1k commented Jun 11, 2015

@egavrin, fix some notes and you'll be included ;))

@ruben-ayrapetyan
Copy link
Contributor Author

@galpeter, I've updated pull request according to your notes, except about missing comments to arguments (as I'm waiting for your answer).
Please, review.

galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
tests/jerry/eval.js
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've got a test case which fails with this PR:

var r = function() { return eval('return 1;'); };
try {
    r();
    print("fail");
    assert(false);
} catch (e) {
    print(e);
    assert(e instanceof SyntaxError);
}

It prints out the fail message, but a SyntaxError should be thrown. Other engines reports the error with the "return not in function" message.

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 the case.
Correct syntax errors handling would be implemented in context of #54.
Could you, please, add the test case to the issue?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay

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

@galpeter galpeter mentioned this pull request Jun 11, 2015
Closed
14 tasks
zherczeg
zherczeg reviewed Jun 12, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-function.cpp
#include "ecma-conversion.h"
#include "ecma-exceptions.h"
#include "ecma-try-catch-macro.h"
Copy link
Member

Choose a reason for hiding this comment

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

In other files, these are lexically ordered. This rule was true here before the change.

I don't mind if we discard this rule, just we should say it explicitly.

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, there is an issue. Will fix.

@galpeter
Copy link
Contributor

I've got no other comments, lgtm

@ruben-ayrapetyan ruben-ayrapetyan force-pushed the eval_final_dev branch 2 times, most recently from 6d76992 to adf7262 Compare June 12, 2015 09:08
@ruben-ayrapetyan
Copy link
Contributor Author

@zherczeg, thank you for your notes.

The pull request is updated.

@ruben-ayrapetyan ruben-ayrapetyan force-pushed the eval_final_dev branch 3 times, most recently from b539076 to 70e2fe9 Compare June 13, 2015 15:01
sand1k and others added 3 commits June 13, 2015 18:01
@sand1k @ruben-ayrapetyan
Add support for eval in parser.
3577261 
JerryScript-DCO-1.0-Signed-off-by: Andrey Shitov a.shitov@samsung.com
@ruben-ayrapetyan
Support of multiple bytecode arrays in interpreter and ecma-runtime.
8ea0e23 
JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Extracting part of ecma_op_eval to ecma_op_eval_chars_buffer that wit…
534ee42 
…h 'code' argument represented as array of characters instead of ecma-string.

Updating jerry_api_eval to use ecma_op_eval_chars_buffer, so removing redundant conversion of character array to ecma-string.

JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Adding interpreter invocation to ecma_op_eval_chars_buffer.
d18de24 
JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Implementing [[Construct]] of built-in Function object.
6fadcd3 
JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Replacing 'this_arg' meta opcode with more general 'call site additio…
446e8bf 
…nal information' meta opcode with flags describing the call site features, and, optionally, 'this' argument.

Introducing opcode_call_flags_t for argument of the new meta opcode, currently with two flags: 'have this argument' and '"direct call to eval" form'.

JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Introducing interpreter context flag, indicating if there is call of …
b310d76 
…'Direct call to eval' form in process, and interfaces for accessing the flag and 'strict mode' flag.

JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan
Implementing 'eval' routine of built-in Global object.
5d5e75f 
JerryScript-DCO-1.0-Signed-off-by: Ruben Ayrapetyan r.ayrapetyan@samsung.com
@ruben-ayrapetyan ruben-ayrapetyan merged commit 5d5e75f into master Jun 13, 2015
@ruben-ayrapetyan ruben-ayrapetyan mentioned this pull request Jun 13, 2015
Closed
@ruben-ayrapetyan ruben-ayrapetyan deleted the eval_final_dev branch June 13, 2015 16:06
This was referenced May 17, 2020
Closed
Closed
This was referenced Oct 20, 2020
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
critical Raises security concerns development Feature implementation ecma builtins Related to ECMA built-in routines ecma core Related to core ECMA functionality parser Related to the JavaScript parser
Projects
None yet
Milestone
Core ECMA features
Development

Successfully merging this pull request may close these issues.
5 participants
@ruben-ayrapetyan @egavrin @sand1k @galpeter @zherczeg