Reduce code duplication between Array.reduce and reduceRight #2280
Conversation
|
This reduces the binary size by about 2000 bytes on my machine (libjerry-core.a: 3301510 ->3299494). |
mmatyas
force-pushed
the
builtin_codemin_3
branch
from
218e2c1 to
afbe87a
Compare
|
Rebased to master to fix the Travis build. |
|
|
||
| /* 8.b */ | ||
| while (!k_present && index < len && ecma_is_value_empty (ret_value)) | ||
| bool has_items_left = start_from_left ? (index < len) : (index != UINT32_MAX); |
There was a problem hiding this comment.
has_items_left ? I also don't like the UINT32_MAX comparison, sometimes the compilers decides that certain overflowed values cannot happen.
There was a problem hiding this comment.
The original code for reduceRight used an int64_t index to handle the case where the index is 0 and reduced by 1. I've used a uint32 for both cases, and compare with UINT32_MAX (ie. -1) instead. Would you prefer using int64_t?
There was a problem hiding this comment.
Would it be possible to use an indexing from right? So go from 0 - len, and substract the current value from the len? I would not use int64 because it is not a native type on 32 bit systems.
There was a problem hiding this comment.
Actually the whole has_items_left could be replaced with a counter -- I'll push an update.
|
|
||
| /* 8.b.iv */ | ||
| index++; | ||
| index = start_from_left ? (index + 1) : (index - 1); |
There was a problem hiding this comment.
You could have a direction or something similar.
There was a problem hiding this comment.
You mean using a direction type, something like enum { LEFT, RIGHT } ?
mmatyas
force-pushed
the
builtin_codemin_3
branch
from
afbe87a to
6aa16f6
Compare
|
|
||
| /* 6. */ | ||
| uint32_t index = 0; | ||
| uint32_t index = start_from_left ? 0 : (len - 1); |
There was a problem hiding this comment.
Ok my idea looks like the following. This variable is always initialized with 0.
uint32_t index = 0;
|
|
||
| /* 8.b */ | ||
| while (!k_present && index < len && ecma_is_value_empty (ret_value)) | ||
| while (!k_present && items_visited < len && ecma_is_value_empty (ret_value)) |
There was a problem hiding this comment.
This is always index < len, just like in the original.
| ECMA_FINALIZE (current_value); | ||
|
|
||
| /* 8.b.iv */ | ||
| index++; |
| while (!k_present && items_visited < len && ecma_is_value_empty (ret_value)) | ||
| { | ||
| /* 8.b.i */ | ||
| ecma_string_t *index_str_p = ecma_new_ecma_string_from_uint32 (index); |
There was a problem hiding this comment.
And here comes the catch: depending on start_from_left we read index or len - 1 - index. We can create a local variable for len - 1.
There was a problem hiding this comment.
This way no need int64 magic, no need to handle negative numbers.
mmatyas
force-pushed
the
builtin_codemin_3
branch
from
6aa16f6 to
ebf09e6
Compare
|
@zherczeg updated, what do you think? |
| { | ||
| /* 8.b.i */ | ||
| ecma_string_t *index_str_p = ecma_new_ecma_string_from_uint32 (index); | ||
| ecma_string_t *index_str_p = ecma_new_ecma_string_from_uint32 (start_from_left |
There was a problem hiding this comment.
This is not the correct style.
(a ? b
: c)
| ecma_value_t current_index; | ||
|
|
||
| for (; index < len && ecma_is_value_empty (ret_value); index++) | ||
| while (index < len && ecma_is_value_empty (ret_value)) |
There was a problem hiding this comment.
Is it a personal preference to remove the for or something else?
There was a problem hiding this comment.
It was changed due to the index update in the previous versions. With that part reverted, the for might be a better choice indeed.
Their code differs only in handling the array index. JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu
mmatyas
force-pushed
the
builtin_codemin_3
branch
from
ebf09e6 to
b2f1e39
Compare
|
LGTM |
…ript-project#2280) Their code differs only in handling the array index. JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu
4 participants
Their code differs only in handling the array index.
JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu