Skip to content

Implement String.prototype.replace function. #474

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
Closed
zherczeg wants to merge 1 commit into from
Closed

Implement String.prototype.replace function. #474

zherczeg wants to merge 1 commit into from

Conversation

@zherczeg
Copy link
Member

@zherczeg zherczeg commented Jul 27, 2015

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg@inf.u-szeged.hu

@galpeter galpeter added the ecma builtins Related to ECMA built-in routines label Jul 27, 2015
@galpeter galpeter added this to the ECMA builtins milestone Jul 27, 2015
@ILyoan ILyoan mentioned this pull request Jul 27, 2015
Closed
25 tasks
@egavrin egavrin assigned galpeter and ruben-ayrapetyan and unassigned galpeter Jul 28, 2015
@zherczeg zherczeg force-pushed the string-prototype-replace-dev branch from 5aa7e40 to 4d4c4f8 Compare July 28, 2015 13:50
ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 29, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp Outdated
Copy link

@ruben-ayrapetyan ruben-ayrapetyan Jul 29, 2015

Choose a reason for hiding this comment

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

The variable name is very similar to argument's name appended_string_p. Maybe, ret_string_p would be better.

@ruben-ayrapetyan
Copy link

ruben-ayrapetyan commented Jul 29, 2015

Could you, please, add note to ecma_builtin_string_prototype_object_replace comment that would briefly describe scheme of replacement (sequence of actions) with references to helper functions?

@zherczeg zherczeg force-pushed the string-prototype-replace-dev branch 2 times, most recently from d5442ba to f8a55f7 Compare July 29, 2015 09:54
@zherczeg
Copy link
Member Author

zherczeg commented Jul 29, 2015

I think I did the requested changes.

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

@ruben-ayrapetyan ruben-ayrapetyan Jul 29, 2015

Choose a reason for hiding this comment

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

The condition is always true.

@ruben-ayrapetyan
Copy link

ruben-ayrapetyan commented Jul 29, 2015

@zherczeg, thank you for the update!

After updating #474 (comment), #474 (comment), the changes would look good to me.

@zherczeg zherczeg force-pushed the string-prototype-replace-dev branch from f8a55f7 to b82664b Compare July 29, 2015 11:35
@zherczeg
Copy link
Member Author

zherczeg commented Jul 29, 2015

Thank you for the review. I added two new tests.

@zherczeg
Implement String.prototype.replace function.
f1c178c 
JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg@inf.u-szeged.hu
@zherczeg zherczeg force-pushed the string-prototype-replace-dev branch from b82664b to f1c178c Compare July 29, 2015 11:50
@galpeter
Copy link
Contributor

galpeter commented Jul 29, 2015

lgtm

@zherczeg zherczeg assigned egavrin and unassigned ruben-ayrapetyan Jul 29, 2015
@ruben-ayrapetyan
Copy link

ruben-ayrapetyan commented Jul 29, 2015

Looks good to me

@egavrin
Copy link
Contributor

egavrin commented Jul 30, 2015

make push

@egavrin egavrin assigned zherczeg and unassigned egavrin Jul 30, 2015
@zherczeg
Copy link
Member Author

zherczeg commented Jul 30, 2015

Landed 048e209

@zherczeg zherczeg closed this Jul 30, 2015
@zherczeg zherczeg deleted the string-prototype-replace-dev branch July 30, 2015 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@zherczeg zherczeg

Labels

ecma builtins Related to ECMA built-in routines

Projects

None yet

Milestone

ECMA builtins

Development

Successfully merging this pull request may close these issues.

5 participants

@zherczeg @ruben-ayrapetyan @galpeter @egavrin