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.

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 10, 2015
Merged

Implement Date.prototype.toString #336

merged 1 commit into from
Jul 10, 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 8, 2015
@LaszloLango LaszloLango added this to the ECMA builtins milestone Jul 8, 2015
egavrin
egavrin reviewed Jul 8, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
*/
static void
date_insert_leading_zeros (ecma_string_t **str_p, /**< input/output string */
ecma_number_t num, /**< input number */
Copy link
Contributor

Choose a reason for hiding this comment

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

Broken formatting

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, prefix should be ecma_date_.

@LaszloLango LaszloLango force-pushed the date_tostring branch 2 times, most recently from 65c6ee3 to cfb64fa Compare July 8, 2015 13:30
egavrin
egavrin reviewed Jul 8, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
* 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 ^_^

@egavrin
Copy link
Contributor

egavrin commented Jul 8, 2015

Good to me

ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 8, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
*str_p = concat_p;
}

} /* insert_leading_zeros */
Copy link
Contributor

Choose a reason for hiding this comment

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

Incorrect comment

@galpeter galpeter mentioned this pull request Jul 8, 2015
Closed
50 tasks
@kkristof
Copy link
Contributor

kkristof commented Jul 9, 2015

lgtm

@egavrin egavrin assigned ruben-ayrapetyan and unassigned egavrin Jul 9, 2015
ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 9, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
{
div /= 10;

ecma_string_t *zero_str_p = ecma_new_ecma_string_from_number (ECMA_NUMBER_ZERO);
Copy link
Contributor

Choose a reason for hiding this comment

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

ecma_new_ecma_string_from_uint32 would be better in the case, as no additional pools chunk would be allocated for floating point value.

@LaszloLango
Implement Date.prototype.toString
9b24d8f 
JerryScript-DCO-1.0-Signed-off-by: László Langó llango.u-szeged@partner.samsung.com
@LaszloLango LaszloLango merged commit 9b24d8f into jerryscript-project:master Jul 10, 2015
@LaszloLango LaszloLango mentioned this pull request Jul 10, 2015
Closed
ruben-ayrapetyan
ruben-ayrapetyan reviewed Jul 10, 2015
View reviewed changes
jerry-core/ecma/builtin-objects/ecma-builtin-date-prototype.cpp
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.

@ruben-ayrapetyan ruben-ayrapetyan removed their assignment Jul 10, 2015
@LaszloLango LaszloLango deleted the date_tostring branch December 14, 2015 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@LaszloLango LaszloLango

Labels
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.
4 participants
@LaszloLango @egavrin @kkristof @ruben-ayrapetyan