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.

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

Merged
merged 1 commit into from
Jul 7, 2015

Conversation

LaszloLango
Copy link
Contributor

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com

@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
"2015-01-01T00:00+01:00Z", "2015/01/01", "2015-01-32", "2015--1",
"2015-13", "2015-01--1", "-215", "-215-01-01", "2015-01-00",
"2015-01-01T25:00", "2015-01-01T00:60", "2015-01-01T-1:00",
"2015-01-01T00:-1"];
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

&& *date_char_p == '-')
{
/* eat up '-' */
date_char_p += sizeof (lit_utf8_byte_t);
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.

ecma_number_t time = ECMA_NUMBER_ZERO;

/* 2. read month if any */
if (!lit_utf8_iterator_is_eos (&iter)
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

*/
static ecma_number_t
ecma_date_parse_date_chars (lit_utf8_iterator_t *iter, /**< iterator of the utf8 string */
uint32_t num_of_chars) /**< number of characters to read and convert */
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


if (copy_size > 0)
{
ecma_string_t *str_p = ecma_new_ecma_string_from_utf8 (str_start_p, copy_size);
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

JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
@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
50 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
critical Raises security concerns development Feature implementation ecma builtins Related to ECMA built-in routines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants