-
Notifications
You must be signed in to change notification settings - Fork 684
Optimising the ToString operation #1098
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
Optimising the ToString operation #1098
Conversation
res_p = ecma_new_ecma_string_from_uint32 ((uint32_t) (ecma_get_integer_from_value (value))); | ||
} | ||
} | ||
else if (ecma_is_value_float_number (value)) |
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 would call ecma_get_integer_from_value (value) first, then check the sign, and that makes ecma_get_number_from_value call unnecessary.
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.
Yes, indeed, thanks! I've updated this patch.
else if (ecma_is_value_integer_number (value)) | ||
{ | ||
ecma_integer_value_t num = ecma_get_integer_from_value (value); | ||
if (num < 0) |
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 would add a newline before this. After that LGTM.
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.
Added, thanks.
@@ -352,10 +352,23 @@ ecma_op_to_string (ecma_value_t value) /**< ecma value */ | |||
|
|||
if (ecma_is_value_string (value)) | |||
{ | |||
res_p = ecma_get_string_from_value (value); | |||
res_p = ecma_get_string_from_value (value);ecma-array-object.c |
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.
accident change?
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.
Sadly, fixed.
LGTM |
1 similar comment
LGTM |
Create new ecma-string from positive integers without cast it to ecma_number JerryScript-DCO-1.0-Signed-off-by: Robert Sipka [email protected]
rebased with master |
Create new ecma-string from positive integers without cast it to ecma_number
JerryScript-DCO-1.0-Signed-off-by: Robert Sipka [email protected]