-
Notifications
You must be signed in to change notification settings - Fork 684
Implement Date.prototype.toString #336
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
#include "ecma-helpers.h" | ||
#include "ecma-objects.h" | ||
#include "ecma-try-catch-macro.h" | ||
#include "fdlibm-math.h" | ||
|
||
#ifndef CONFIG_ECMA_COMPACT_PROFILE_DISABLE_DATE_BUILTIN | ||
|
||
|
@@ -41,6 +42,53 @@ | |
* @{ | ||
*/ | ||
|
||
/** | ||
* Insert leading zeros to a string of a number if needed. | ||
*/ | ||
static void | ||
ecma_date_insert_leading_zeros (ecma_string_t **str_p, /**< input/output string */ | ||
ecma_number_t num, /**< input number */ | ||
uint32_t length) /**< length of string of number */ | ||
{ | ||
JERRY_ASSERT (length >= 1); | ||
|
||
/* If the length is bigger than the number of digits in num, then insert leding zeros. */ | ||
for (uint32_t i = length - 1; i > 0 && num < pow (10,i); i--) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to perform There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you suggest instead? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The following is not the best solution, and can be improved, but uint32_t first_index = length - 1u;
ecma_number_t power_i = (ecma_number_t) pow (10, first_index);
for (uint32_t i = first_index; i > 0 && num < power_i; i--, power_i /= 10)
{
ecma_string_t *zero_str_p = ecma_new_ecma_string_from_uint32 (0);
ecma_string_t *concat_p = ecma_concat_ecma_strings (zero_str_p, *str_p);
ecma_deref_ecma_string (zero_str_p);
ecma_deref_ecma_string (*str_p);
*str_p = concat_p;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I'll do it. Is it good for you if I add this change to #360? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is better to open separate pull requests for unrelated changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is also using these two helper functions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok. |
||
{ | ||
ecma_string_t *zero_str_p = ecma_new_ecma_string_from_uint32 (0); | ||
ecma_string_t *concat_p = ecma_concat_ecma_strings (zero_str_p, *str_p); | ||
ecma_deref_ecma_string (zero_str_p); | ||
ecma_deref_ecma_string (*str_p); | ||
*str_p = concat_p; | ||
} | ||
} /* ecma_date_insert_leading_zeros */ | ||
|
||
/** | ||
* Insert a number to the start of a string with a specific separator character and | ||
* fix length. If the length is bigger than the number of digits in num, then insert leding zeros. | ||
*/ | ||
static void | ||
ecma_date_insert_num_with_sep (ecma_string_t **str_p, /**< input/output string */ | ||
ecma_number_t num, /**< input number */ | ||
lit_magic_string_id_t magic_str_id, /**< separator character id */ | ||
uint32_t length) /**< length of string of number */ | ||
{ | ||
ecma_string_t *magic_string_p = ecma_get_magic_string (magic_str_id); | ||
|
||
ecma_string_t *concat_p = ecma_concat_ecma_strings (magic_string_p, *str_p); | ||
ecma_deref_ecma_string (magic_string_p); | ||
ecma_deref_ecma_string (*str_p); | ||
*str_p = concat_p; | ||
|
||
ecma_string_t *num_str_p = ecma_new_ecma_string_from_number (num); | ||
concat_p = ecma_concat_ecma_strings (num_str_p, *str_p); | ||
ecma_deref_ecma_string (num_str_p); | ||
ecma_deref_ecma_string (*str_p); | ||
*str_p = concat_p; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not to use There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In this case we don't need a temporary variable on the call site (like using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, we can perform the call without temporary variable on the call site: output_str_p = ecma_date_insert_num_with_sep (output_str_p,
seconds,
LIT_MAGIC_STRING_DOT_CHAR,
2); Difference in the case is that change of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ruben-ayrapetyan, I see. So we can dereference the string in the function and return the you new pointer. Do you mean like this? static emca_string_t *str_p
ecma_date_insert_num_with_sep (ecma_string_t *str_p, /**< input string */
ecma_number_t num, /**< input number */
lit_magic_string_id_t magic_str_id, /**< separator character id */
uint32_t length) /**< length of string of number */
{
ecma_string_t *magic_string_p = ecma_get_magic_string (magic_str_id);
ecma_string_t *concat_p = ecma_concat_ecma_strings (magic_string_p, str_p);
ecma_deref_ecma_string (magic_string_p);
ecma_deref_ecma_string (str_p);
str_p = concat_p;
ecma_string_t *num_str_p = ecma_new_ecma_string_from_number (num);
concat_p = ecma_concat_ecma_strings (num_str_p, str_p);
ecma_deref_ecma_string (num_str_p);
ecma_deref_ecma_string (str_p);
return concat_p;
} May we deref the parameter in this function? Isn't it dangerous? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LaszloLango, could you, please, clarify what dereferencing do you mean? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ruben-ayrapetyan, what makes me unsure is that if we want to use it like this: output_str_p = ecma_date_insert_num_with_sep (output_str_p,
seconds,
LIT_MAGIC_STRING_DOT_CHAR,
2); then ecma_date_insert_num_with_sep (output_str_p,
seconds,
LIT_MAGIC_STRING_DOT_CHAR,
2); then There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @LaszloLango, I see. I think that the approach that you've described is good for now, until the hidden dereference issue wouldn't be solved somehow (maybe, after switching to C++ in the whole engine, it would be solved in a simple way). |
||
|
||
ecma_date_insert_leading_zeros (str_p, num, length); | ||
} /* ecma_date_insert_num_with_sep */ | ||
|
||
/** | ||
* The Date.prototype object's 'toString' routine | ||
* | ||
|
@@ -53,7 +101,65 @@ | |
static ecma_completion_value_t | ||
ecma_builtin_date_prototype_to_string (ecma_value_t this_arg) /**< this argument */ | ||
{ | ||
ECMA_BUILTIN_CP_UNIMPLEMENTED (this_arg); | ||
ecma_completion_value_t ret_value = ecma_make_empty_completion_value (); | ||
|
||
if (ecma_object_get_class_name (ecma_get_object_from_value (this_arg)) != LIT_MAGIC_STRING_DATE_UL) | ||
{ | ||
ret_value = ecma_raise_type_error ("Incomplete Date type"); | ||
} | ||
else | ||
{ | ||
ECMA_TRY_CATCH (obj_this, | ||
ecma_op_to_object (this_arg), | ||
ret_value); | ||
|
||
ecma_object_t *obj_p = ecma_get_object_from_value (obj_this); | ||
ecma_property_t *prim_value_prop_p; | ||
prim_value_prop_p = ecma_get_internal_property (obj_p, ECMA_INTERNAL_PROPERTY_PRIMITIVE_NUMBER_VALUE); | ||
ecma_number_t *prim_value_num_p = ECMA_GET_NON_NULL_POINTER (ecma_number_t, | ||
prim_value_prop_p->u.internal_property.value); | ||
|
||
if (ecma_number_is_nan (*prim_value_num_p)) | ||
{ | ||
ecma_string_t *magic_str_p = ecma_get_magic_string (LIT_MAGIC_STRING_INVALID_DATE_UL); | ||
ret_value = ecma_make_normal_completion_value (ecma_make_string_value (magic_str_p)); | ||
} | ||
else | ||
{ | ||
ecma_number_t milliseconds = ecma_date_ms_from_time (*prim_value_num_p); | ||
ecma_string_t *output_str_p = ecma_new_ecma_string_from_number (milliseconds); | ||
ecma_date_insert_leading_zeros (&output_str_p, milliseconds, 3); | ||
|
||
ecma_number_t seconds = ecma_date_sec_from_time (*prim_value_num_p); | ||
ecma_date_insert_num_with_sep (&output_str_p, seconds, LIT_MAGIC_STRING_DOT_CHAR, 2); | ||
|
||
ecma_number_t minutes = ecma_date_min_from_time (*prim_value_num_p); | ||
ecma_date_insert_num_with_sep (&output_str_p, minutes, LIT_MAGIC_STRING_COLON_CHAR, 2); | ||
|
||
ecma_number_t hours = ecma_date_hour_from_time (*prim_value_num_p); | ||
ecma_date_insert_num_with_sep (&output_str_p, hours, LIT_MAGIC_STRING_COLON_CHAR, 2); | ||
|
||
ecma_number_t day = ecma_date_date_from_time (*prim_value_num_p); | ||
ecma_date_insert_num_with_sep (&output_str_p, day, LIT_MAGIC_STRING_TIME_SEP_U, 2); | ||
|
||
/* | ||
* Note: | ||
* 'ecma_date_month_from_time' (ECMA 262 v5, 15.9.1.4) returns a number from 0 to 11, | ||
* but we have to print the month from 1 to 12 for ISO 8601 standard (ECMA 262 v5, 15.9.1.15). | ||
*/ | ||
ecma_number_t month = ecma_date_month_from_time (*prim_value_num_p) + 1; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why we have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add comment for that |
||
ecma_date_insert_num_with_sep (&output_str_p, month, LIT_MAGIC_STRING_MINUS_CHAR, 2); | ||
|
||
ecma_number_t year = ecma_date_year_from_time (*prim_value_num_p); | ||
ecma_date_insert_num_with_sep (&output_str_p, year, LIT_MAGIC_STRING_MINUS_CHAR, 4); | ||
|
||
ret_value = ecma_make_normal_completion_value (ecma_make_string_value (output_str_p)); | ||
} | ||
|
||
ECMA_FINALIZE (obj_this); | ||
} | ||
|
||
return ret_value; | ||
} /* ecma_builtin_date_prototype_to_string */ | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
// Copyright 2015 Samsung Electronics Co., Ltd. | ||
// Copyright 2015 University of Szeged. | ||
// | ||
// Licensed under the Apache License, Version 2.0 (the "License"); | ||
// you may not use this file except in compliance with the License. | ||
// You may obtain a copy of the License at | ||
// | ||
// http://www.apache.org/licenses/LICENSE-2.0 | ||
// | ||
// Unless required by applicable law or agreed to in writing, software | ||
// distributed under the License is distributed on an "AS IS" BASIS | ||
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
// See the License for the specific language governing permissions and | ||
// limitations under the License. | ||
|
||
assert (new Date(NaN) == "Invalid Date"); | ||
assert (new Date("2015-02-13") == "2015-02-13T00:00:00.000"); | ||
assert (new Date("2015-07-08T11:29:05.023") == "2015-07-08T11:29:05.023"); |
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.
@ruben-ayrapetyan why
ecma_date_
shouldn't it beecma_builtin_date_
?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 think
ecma_builtin_date_
will be too long for a prefix and we usedecma_date_
for helpers inecma-builtin-date.cpp
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'm asking because neighbour function is named with
ecma_builtin_date_
: https://github.com/Samsung/jerryscript/pull/336/files#diff-4c3f6b953c0f51c11d2a079e9dcc10f8R103There 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.
@egavrin, I know, but those two functions are not builtins, only helpers.
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.
@egavrin, I think that you are right. Following current naming style, the prefix should be even
ecma_builtin_date_prototype_helper_
. But this prefix is too long. So, probably, naming scheme for built-in helpers should be somehow updated. I'm not sure how exactly. But at least,ecma_date_
, of course, should be added.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.
Let's leave as is ^_^