Skip to content

Implement String.prototype.charCodeAt() #352

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
galpeter merged 1 commit into from
Jul 20, 2015
Merged

Implement String.prototype.charCodeAt() #352

galpeter 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 lvidacs.u-szeged@partner.samsung.com

@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 Outdated
Copy link
Contributor

@egavrin egavrin Jul 9, 2015

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 Outdated
Copy link
Contributor

@galpeter galpeter Jul 9, 2015

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 Outdated
Copy link
Contributor

@galpeter galpeter Jul 15, 2015

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

zherczeg commented Jul 17, 2015

LGTM

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 17, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp Outdated
Copy link

@ruben-ayrapetyan ruben-ayrapetyan Jul 17, 2015

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

ruben-ayrapetyan commented Jul 20, 2015

@lvidacs, thank you for update.

@ruben-ayrapetyan
Copy link

ruben-ayrapetyan commented Jul 20, 2015

@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 lvidacs.u-szeged@partner.samsung.com
@lvidacs lvidacs force-pushed the string_prototype_charcodeat branch from 61a6e93 to c7a47c1 Compare July 20, 2015 14:22
@ruben-ayrapetyan
Copy link

ruben-ayrapetyan commented Jul 20, 2015

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