Skip to content

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

Merged
merged 1 commit into from
Jul 10, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
108 changes: 107 additions & 1 deletion jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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 */
Copy link
Contributor

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 be ecma_builtin_date_?

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 think ecma_builtin_date_ will be too long for a prefix and we used ecma_date_ for helpers in 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.

I'm asking because neighbour function is named with ecma_builtin_date_ : https://github.com/Samsung/jerryscript/pull/336/files#diff-4c3f6b953c0f51c11d2a079e9dcc10f8R103

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor

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 ^_^

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--)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to perform pow on every iteration? It is unnecessarily resource consuming.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you suggest instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

The following is not the best solution, and can be improved, but pow is calculated only once in the version:

 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;
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

It is better to open separate pull requests for unrelated changes.
If the change involves code, updated in the #360, it would be ok, if you would place the change into a separate commit of the pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is also using these two helper functions.

Copy link
Contributor

Choose a reason for hiding this comment

The 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;
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 return for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 ecma_concat_ecma_strings). I think it is easier to use and more readable. Should I change this to return?

Copy link
Contributor

Choose a reason for hiding this comment

The 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 output_str_p value becomes evident, so readability is improved.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

@LaszloLango, could you, please, clarify what dereferencing do you mean?
How is it different from dereferencing in ecma_date_insert_num_with_sep (ecma_string_t **str_p, version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 must deref output_str_p because it returns with a new string pointer and it will cause a memory leak. If we use this function wrong like this:

ecma_date_insert_num_with_sep (output_str_p,
                                              seconds,
                                              LIT_MAGIC_STRING_DOT_CHAR,
                                              2);

then output_str_p will be freed and cannot be used after the call. Maybe I misunderstood you. Is this you are talking about?

Copy link
Contributor

Choose a reason for hiding this comment

The 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).
The code is ok for me.


ecma_date_insert_leading_zeros (str_p, num, length);
} /* ecma_date_insert_num_with_sep */

/**
* The Date.prototype object's 'toString' routine
*
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we have +1 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.

ecma_date_month_from_time (http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.4) returns a number from 0 to 11, but we have to print the month from 1 to 12. (ISO 8601, http://www.ecma-international.org/ecma-262/5.1/#sec-15.9.1.15)

Copy link
Contributor

Choose a reason for hiding this comment

The 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 */

/**
Expand Down
3 changes: 3 additions & 0 deletions jerry-core/lit/lit-magic-strings.inc.h
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_MIN_VALUE_U, "MIN_VALUE")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_POSITIVE_INFINITY_U, "POSITIVE_INFINITY")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_NEGATIVE_INFINITY_U, "NEGATIVE_INFINITY")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_COMPACT_PROFILE_ERROR_UL, "CompactProfileError")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_INVALID_DATE_UL, "Invalid Date")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_APPLY, "apply")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_CALL, "call")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_BIND, "bind")
Expand All @@ -217,6 +218,7 @@ LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_MESSAGE, "message")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_G_CHAR, "g")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_I_CHAR, "i")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_M_CHAR, "m")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_TIME_SEP_U, "T")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_SLASH_CHAR, "/")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_BACKSLASH_CHAR, "\\")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_EMPTY_NON_CAPTURE_GROUP, "(?:)")
Expand All @@ -227,6 +229,7 @@ LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_RIGHT_BRACE_CHAR, "}")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_MINUS_CHAR, "-")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_COLON_CHAR, ":")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_COMMA_CHAR, ",")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_DOT_CHAR, ".")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_DOUBLE_QUOTE_CHAR, "\"")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_NEW_LINE_CHAR, "\n")
LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING_SPACE_CHAR, " ")
Expand Down
18 changes: 18 additions & 0 deletions tests/jerry/date-tostring.js
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");