Implement Date.prototype.toString #336
Conversation
LaszloLango
added
ecma builtins
There was a problem hiding this comment.
Also, prefix should be ecma_date_.
LaszloLango
force-pushed
the
date_tostring
branch
2 times, most recently
from
65c6ee3 to
cfb64fa
Compare
There was a problem hiding this comment.
@ruben-ayrapetyan why ecma_date_ shouldn't it be ecma_builtin_date_?
There was a problem hiding this comment.
I think ecma_builtin_date_ will be too long for a prefix and we used ecma_date_ for helpers in ecma-builtin-date.cpp
There was a problem hiding this comment.
I'm asking because neighbour function is named with ecma_builtin_date_ : https://github.com/Samsung/jerryscript/pull/336/files#diff-4c3f6b953c0f51c11d2a079e9dcc10f8R103
There was a problem hiding this comment.
@egavrin, I know, but those two functions are not builtins, only helpers.
There was a problem hiding this comment.
@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.
|
Good to me |
LaszloLango
force-pushed
the
date_tostring
branch
from
cfb64fa to
7d57c2c
Compare
|
lgtm |
There was a problem hiding this comment.
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
force-pushed
the
date_tostring
branch
from
7d57c2c to
0d865aa
Compare
JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]
LaszloLango
force-pushed
the
date_tostring
branch
from
0d865aa to
9b24d8f
Compare
There was a problem hiding this comment.
Why do we need to perform pow on every iteration? It is unnecessarily resource consuming.
There was a problem hiding this comment.
What do you suggest instead?
There was a problem hiding this comment.
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;
}There was a problem hiding this comment.
Thanks, I'll do it. Is it good for you if I add this change to #360?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is also using these two helper functions.
4 participants
JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]