Skip to content

Implement Date.parse #297

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

Implement Date.parse #297

merged 1 commit into from
Jul 7, 2015

Conversation

LaszloLango
Copy link
Contributor

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

@LaszloLango LaszloLango added ecma builtins Related to ECMA built-in routines development Feature implementation labels Jul 2, 2015
@LaszloLango LaszloLango added this to the ECMA builtins milestone Jul 2, 2015
galpeter
galpeter reviewed Jul 2, 2015
View reviewed changes
tests/jerry/date-parse.js Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have this as one entry per line? Also maybe some other types like undefined, numbers, objects?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 2, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

// Btw, is it correct way to iterate string? Can't the string contain a character, encoded with a multi-byte sequence?

Update:
I see. It is correct in the case.
Please, consider rebasing the pull request on #293, when it would be merged, and updating the code to use utf8 iterators.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@LaszloLango
Copy link
Contributor Author

@galpeter, @ruben-ayrapetyan I've updated the PR. Please check.

galpeter
galpeter reviewed Jul 6, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date.cpp
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need to check the eos in the if cases? Isn't the peek_next enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

First line of peek_next is JERRY_ASSERT (!lit_utf8_iterator_is_eos (iter_p));, so the answer is yes we do. peek_next isn't enough.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, if the peek_next throws an assert if the string is over, then that's not a good thing (in this case)

@galpeter
Copy link
Contributor

galpeter commented Jul 6, 2015

lgtm

@LaszloLango LaszloLango mentioned this pull request Jul 6, 2015
Merged
zherczeg
zherczeg reviewed Jul 6, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date.cpp
Copy link
Member

Choose a reason for hiding this comment

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

This is kind of a maximum for me (max_length) ?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, forget this comment. So hard to read code without early returns.

@LaszloLango
Copy link
Contributor Author

I've updated the PR

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 6, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date.cpp Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not to use ecma_utf8_string_to_number here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't know is there such a function. :) I'll fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok.

@LaszloLango
Copy link
Contributor Author

@ruben-ayrapetyan, I've updated the PR, please check.

@LaszloLango LaszloLango added the critical Raises security concerns label Jul 6, 2015
@ruben-ayrapetyan
Copy link
Contributor

Looks good to me

@LaszloLango
Implement Date.parse
cadc8f4 
JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]
@LaszloLango LaszloLango merged commit cadc8f4 into jerryscript-project:master Jul 7, 2015
@LaszloLango LaszloLango deleted the date_parse_dev branch July 7, 2015 06:56
@galpeter galpeter mentioned this pull request Jul 8, 2015
Closed
50 tasks
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.

5 participants

@LaszloLango @galpeter @ruben-ayrapetyan @zherczeg @egavrin