Skip to content

Implement toLowerCase and toUpperCase built-in functions. #365

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

Implement toLowerCase and toUpperCase built-in functions. #365

wants to merge 1 commit into from

Conversation

zherczeg
Copy link
Member

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

@egavrin egavrin self-assigned this Jul 10, 2015
@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

Please add reference to #323

@egavrin egavrin added this to the ECMA builtins milestone Jul 10, 2015
@egavrin egavrin added ecma builtins Related to ECMA built-in routines development Feature implementation labels Jul 10, 2015
@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

Good to me.

@zherczeg
Copy link
Member Author

What do you mean by adding a reference? Adding a comment in the function? E.g:
TODO ("Needs a proper upper case implementation. See issue #323");

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 10, 2015
View reviewed changes
jerry-core/lit/lit-char-helpers.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

@zherczeg, could you, please, add LIT_ prefix and add describing comment for the definition?

@zherczeg zherczeg force-pushed the to-lower-upper-case-dev branch from fd0d85b to 9636728 Compare July 10, 2015 13:55
@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

@zherczeg I mean commit message:

Implement toLowerCase and toUpperCase built-in functions. 

Related issue: #323

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]

@egavrin
Copy link
Contributor

egavrin commented Jul 10, 2015

fyi

./jerry-core/ecma/builtin-objects/ecma-builtin-string-prototype.cpp:519: error: line is longer than 120 characters

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 10, 2015
View reviewed changes
jerry-core/lit/lit-char-helpers.h Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

@zherczeg, what is the meaning of the value?

@zherczeg zherczeg force-pushed the to-lower-upper-case-dev branch from 9636728 to 6b8e819 Compare July 11, 2015 06:16
@zherczeg
Copy link
Member Author

Thank you for the comments. I hope fixed all of them.

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 13, 2015
View reviewed changes
jerry-core/lit/lit-char-helpers.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

@zherczeg, could you, please, add arguments like buffer_size to the function, and check the condition with JERRY_ASSERT?

@ruben-ayrapetyan
Copy link
Contributor

After adding buffer_size-like arguments (#365 (diff), #365 (comment)) the changes would look good to me.

@zherczeg zherczeg force-pushed the to-lower-upper-case-dev branch from 6b8e819 to 0a6ba71 Compare July 14, 2015 10:18
@ruben-ayrapetyan
Copy link
Contributor

@zherczeg, thank you for update.

By the way, __attr_unused____ seems to be unnecessary for me, as JERRY_ASSERT references variables in both debug and release modes. Are there any issues without the attribute?

@ILyoan ILyoan mentioned this pull request Jul 14, 2015
Closed
25 tasks
@zherczeg
Implement toLowerCase and toUpperCase built-in functions.
b130484 
Related issue: #323

JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg [email protected]
@zherczeg zherczeg force-pushed the to-lower-upper-case-dev branch from 0a6ba71 to b130484 Compare July 14, 2015 12:52
@ruben-ayrapetyan
Copy link
Contributor

Looks good to me

@egavrin
Copy link
Contributor

egavrin commented Jul 14, 2015

make push

@zherczeg
Copy link
Member Author

Landed in 69655f4

@zherczeg zherczeg closed this Jul 14, 2015
@egavrin egavrin deleted the to-lower-upper-case-dev branch July 15, 2015 11:49
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

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

@zherczeg @egavrin @ruben-ayrapetyan