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.

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

Implement String.prototype.charCodeAt() #352

Merged
merged 1 commit into from
Jul 20, 2015
Merged

Implement String.prototype.charCodeAt() #352

merged 1 commit into from
Jul 20, 2015

Conversation

lvidacs
Copy link
Contributor

@lvidacs lvidacs commented Jul 9, 2015

JerryScript-DCO-1.0-Signed-off-by: Laszlo Vidacs [email protected]

@kkristof kkristof added ecma builtins Related to ECMA built-in routines development Feature implementation labels Jul 9, 2015
@kkristof kkristof added this to the ECMA builtins milestone Jul 9, 2015
@ILyoan ILyoan mentioned this pull request Jul 9, 2015
Closed
25 tasks
egavrin
egavrin reviewed Jul 9, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp
ecma_string_t *original_string_p = ecma_get_string_from_value (to_string_val);
const ecma_length_t len = ecma_string_get_length (original_string_p);

ecma_number_t * ret_num = ecma_alloc_number ();
Copy link
Contributor

Choose a reason for hiding this comment

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

*ret_num_p

@egavrin egavrin added the critical Raises security concerns label Jul 9, 2015
@egavrin
Copy link
Contributor

egavrin commented Jul 9, 2015

Good to me

@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch from d251525 to c19989e Compare July 9, 2015 15:23
@lvidacs
Copy link
Contributor Author

lvidacs commented Jul 9, 2015

Thanks, the patch is updated.

galpeter
galpeter reviewed Jul 9, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp
ecma_string_t *original_string_p = ecma_get_string_from_value (to_string_val);
const ecma_length_t len = ecma_string_get_length (original_string_p);

ecma_number_t * ret_num_p = ecma_alloc_number ();
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the space after the *

@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch from c19989e to ae86c39 Compare July 15, 2015 14:36
galpeter
galpeter reviewed Jul 15, 2015
View reviewed changes
tests/jerry/string-prototype-charcodeat.js

// check coercible - undefined
try {
assert(true.charCodeAt());
Copy link
Contributor

Choose a reason for hiding this comment

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

This testcase will not invoke the charCodeAt at all. I think you wanted to write: String.prototype.charCodeAt.call(true)

@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch 3 times, most recently from cb1d234 to 5b7a0c6 Compare July 16, 2015 20:16
@zherczeg
Copy link
Member

LGTM

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 17, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp
else
{
/* 6 */
ecma_char_t new_ecma_char = ecma_string_get_char_at_pos (original_string_p, ecma_number_to_uint32 (index_num));
Copy link
Contributor

Choose a reason for hiding this comment

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

While actual length of strings, currently, can't exceed uint32_t range, value of index_num can be bigger. ToInteger is not equivalent to ToUInt32, as ToUInt32 performs "modulo 2^32" operation on its argument, and ToInteger performs floor.

The code would work correctly, as index_num doesn't exceed len, as checked above.

Could you, please, add an assertion that ecma_number_to_uint32 (index_num) == ecma_number_trunc (index_num) and add comment, describing why ToInteger and ToUInt32 are equivalent in the case?

@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch from 5b7a0c6 to 61a6e93 Compare July 20, 2015 13:51
@lvidacs
Copy link
Contributor Author

lvidacs commented Jul 20, 2015

@ruben-ayrapetyan thanks for the comments, the patch is updated

@ruben-ayrapetyan
Copy link
Contributor

@lvidacs, thank you for update.

@ruben-ayrapetyan
Copy link
Contributor

@lvidacs, after fixing #352 (comment) the changes would look good to me

@lvidacs
Implement String.prototype.charCodeAt()
c7a47c1 
JerryScript-DCO-1.0-Signed-off-by: Laszlo Vidacs [email protected]
@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch from 61a6e93 to c7a47c1 Compare July 20, 2015 14:22
@ruben-ayrapetyan
Copy link
Contributor

Looks good to me

@ruben-ayrapetyan ruben-ayrapetyan assigned egavrin and unassigned lvidacs Jul 20, 2015
@egavrin
Copy link
Contributor

egavrin commented Jul 20, 2015

make push

@galpeter galpeter merged commit c7a47c1 into jerryscript-project:master Jul 20, 2015
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
Projects
None yet
Milestone
ECMA builtins
Development

Successfully merging this pull request may close these issues.
6 participants
@lvidacs @egavrin @zherczeg @ruben-ayrapetyan @galpeter @kkristof