Skip to content

Conversation

@dbatyai
Copy link
Member

@dbatyai dbatyai commented Jul 23, 2015

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com

@dbatyai dbatyai added bug Undesired behaviour ecma builtins Related to ECMA built-in routines labels Jul 23, 2015
@dbatyai dbatyai added this to the ECMA builtins milestone Jul 23, 2015

Choose a reason for hiding this comment

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

@dbatyai, the error should be raised in ecma_op_array_object_define_own_property. Is there any case, where it isn't raised?

…ger than UINT_MAX

JerryScript-DCO-1.0-Signed-off-by: Dániel Bátyai dbatyai.u-szeged@partner.samsung.com
@dbatyai dbatyai force-pushed the array_push_max_length branch from 8f8bda2 to e647b4c Compare July 24, 2015 09:08
@dbatyai
Copy link
Member Author

dbatyai commented Jul 24, 2015

@ruben-ayrapetyan, I've updated the patch. The error does get raised correctly in ecma_op_array_object_define_own_property when it should.

Choose a reason for hiding this comment

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

It is better to use ecma_new_ecma_string_from_uint32 instead of ecma_new_ecma_string_from_number, as it reduces memory usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use ecma_new_ecma_string_from_uint32 here, because the index can go above 2^32.

Choose a reason for hiding this comment

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

I see. Thanks for explanation.

@ruben-ayrapetyan
Copy link

Looks good to me

@galpeter
Copy link
Contributor

Looks good to me.

@galpeter
Copy link
Contributor

Landed: fec5933

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Undesired behaviour ecma builtins Related to ECMA built-in routines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants