Skip to content

String.prototype.concat() #159

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

String.prototype.concat() #159

lvidacs wants to merge 1 commit into from

Conversation

@lvidacs
Copy link
Contributor

@lvidacs lvidacs commented Jun 8, 2015

JerryScript-DCO-1.0-Signed-off-by: Laszlo Vidacs lvidacs.u-szeged@partner.samsung.com

@ILyoan ILyoan mentioned this pull request Jun 8, 2015
Closed
25 tasks
galpeter
galpeter reviewed Jun 8, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp Outdated
Copy link
Contributor

@galpeter galpeter Jun 8, 2015

Choose a reason for hiding this comment

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

AFAIK: there is already a magic string for this, we should use that.

Copy link
Contributor

@galpeter galpeter Jun 8, 2015

Choose a reason for hiding this comment

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

Also I think that the ecma_op_to_string correctly returns the "undefined" string if the argument is really undefined. So no need to check the value here.

@egavrin egavrin self-assigned this Jun 8, 2015
@egavrin egavrin added this to the ECMA builtins milestone Jun 8, 2015
@egavrin egavrin added ecma builtins Related to ECMA built-in routines development Feature implementation labels Jun 8, 2015
@lvidacs lvidacs force-pushed the string_prototype_concat branch from 81a8762 to 4d46c9b Compare June 9, 2015 13:26
@lvidacs
Copy link
Contributor Author

lvidacs commented Jun 9, 2015

The patch is updated.

galpeter
galpeter reviewed Jun 9, 2015
View reviewed changes
tests/jerry/string_prototype_concat.js Outdated
Copy link
Contributor

@galpeter galpeter Jun 9, 2015

Choose a reason for hiding this comment

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

This does not test what you want, because the access of the x will throw the Reference error before the concat was even called

@lvidacs lvidacs force-pushed the string_prototype_concat branch from 4d46c9b to ca74ae7 Compare June 9, 2015 13:48
galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
tests/jerry/string_prototype_concat.js
Copy link
Contributor

@galpeter galpeter Jun 11, 2015

Choose a reason for hiding this comment

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

We would need one more test where the arguments of the concat throws an error when the to_string is called.

@lvidacs lvidacs force-pushed the string_prototype_concat branch from ca74ae7 to 4e2a81a Compare June 11, 2015 15:20
@lvidacs
Copy link
Contributor Author

lvidacs commented Jun 11, 2015

Tests added and the code is fixed, thanks for the feedback.

galpeter
galpeter reviewed Jun 11, 2015
View reviewed changes
tests/jerry/string_prototype_concat.js Outdated
Copy link
Contributor

@galpeter galpeter Jun 11, 2015

Choose a reason for hiding this comment

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

This should be an assert instead

@lvidacs
String.prototype.concat()
1850cfb 
JerryScript-DCO-1.0-Signed-off-by: Laszlo Vidacs lvidacs.u-szeged@partner.samsung.com
@lvidacs lvidacs force-pushed the string_prototype_concat branch from 4e2a81a to 1850cfb Compare June 11, 2015 16:50
@galpeter
Copy link
Contributor

galpeter commented Jun 12, 2015

lgtm

@egavrin
Copy link
Contributor

egavrin commented Jun 12, 2015

Good to me, make push

@egavrin egavrin assigned lvidacs and unassigned egavrin Jun 12, 2015
@galpeter
Copy link
Contributor

galpeter commented Jun 12, 2015

Rebase & merged: 72eb14f

@galpeter galpeter closed this Jun 12, 2015
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

@lvidacs lvidacs

Labels

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.

3 participants

@lvidacs @galpeter @egavrin