-
Notifications
You must be signed in to change notification settings - Fork 683
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
Implement Date.parse #297
Conversation
"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"]; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok
65cef6b
to
2684642
Compare
@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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
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 */ |
There was a problem hiding this comment.
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) ?
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
@ruben-ayrapetyan, I've updated the PR, please check. |
Looks good to me |
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
1e053ec
to
cadc8f4
Compare
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com