Reduce code duplication between Array.forEach, some, and every #2275
Conversation
| * @return ecma value | ||
| * Returned value must be freed with ecma_free_value. | ||
| * If the argument is convertible to boolean true, returns ECMA_VALUE_TRUE, | ||
| * otherwise returns ECMA_VALUE_EMPTY. |
There was a problem hiding this comment.
To specify the return value we usually use the @returns element in the docs. (please see a method above)
| ecma_builtin_array_prototype_object_every (ecma_value_t this_arg, /**< this argument */ | ||
| ecma_value_t arg1, /**< callbackfn */ | ||
| ecma_value_t arg2) /**< thisArg */ | ||
| ecma_builtin_convert_true_maybe (ecma_value_t var) |
There was a problem hiding this comment.
No comment for the input argument.
| * If the argument is convertible to boolean false, returns ECMA_VALUE_FALSE, | ||
| * otherwise returns ECMA_VALUE_EMPTY. | ||
| */ | ||
| static ecma_value_t |
There was a problem hiding this comment.
The comments are the same as above.
| * Applies the provided function to each element of the array as long as | ||
| * the return value stays undefined. If the return value stays undefined | ||
| * after processing the whole array, a default value can be returned. | ||
| */ |
There was a problem hiding this comment.
Just like above, the @return should be used.
| * after processing the whole array, a default value can be returned. | ||
| */ | ||
| static ecma_value_t | ||
| ecma_builtin_array_apply (ecma_value_t this_arg, /**< this argument */ |
There was a problem hiding this comment.
In this function the comments like: /* 1. */ are referring to which part of the standard? We should mention that in the comments of the function (just like in the every/some/etc.) case.
| ecma_value_t arg1, /**< callbackfn */ | ||
| ecma_value_t arg2, /**< thisArg */ | ||
| ecma_value_t (*check_stop_cb)(ecma_value_t), /**< retval update fn */ | ||
| ecma_value_t default_retval /**< default if empty */ ) |
There was a problem hiding this comment.
IMHO, I would avoid using the callback method to do the work. What we could do is to only pass an enum value (something like: ARRAY_EVERY, _SOME, _FOR_EACH) and based on this value we can calculate the stop and default return values.
|
|
||
| /** | ||
| * Applies the provided function to each element of the array as long as | ||
| * the return value stays undefined. If the return value stays undefined |
There was a problem hiding this comment.
In the comment the undefined should be empty, as each callback returns ECMA_VALUE_EMPTY. The empty value is different from the undefined.
mmatyas
force-pushed
the
builtin_codemin
branch
2 times, most recently
from
6807858 to
4775884
Compare
|
Thanks, I've updated the patch to use a routine mode enum. |
mmatyas
force-pushed
the
builtin_codemin
branch
from
4775884 to
bf1c380
Compare
|
Rebased to master to fix the Travis build. |
rerobika
left a comment
rerobika
left a comment
There was a problem hiding this comment.
Nice patch! I have just one remark.
| if (ecma_op_to_boolean (call_value)) | ||
| { | ||
| ret_value = ECMA_VALUE_TRUE; | ||
| } |
There was a problem hiding this comment.
This if structure can be merged like this:
if (mode == ARRAY_ROUTINE_EVERY || mode == ARRAY_ROUTINE_SOME)
{
ret_value = ecma_op_to_boolean (call_value) ? ECMA_VALUE_TRUE : ECMA_VALUE_FALSE;
}Therefore ecma_op_to_boolean (call_value) is performed only once in both cases.
There was a problem hiding this comment.
Nice catch, thanks!
There was a problem hiding this comment.
Ah there's a small issue actually, there are cases when the ret_value shouldn't change. The if structure can still be reduced to
if (mode == ARRAY_ROUTINE_EVERY && !ecma_op_to_boolean (call_value))
{
ret_value = ECMA_VALUE_FALSE;
}
else if (mode == ARRAY_ROUTINE_SOME && ecma_op_to_boolean (call_value))
{
ret_value = ECMA_VALUE_TRUE;
}There was a problem hiding this comment.
Ohh, I see. Still looks better now.
Their implementation only differs in the stop condition and the final return value. JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu
mmatyas
force-pushed
the
builtin_codemin
branch
from
bf1c380 to
eedeeff
Compare
…script-project#2275) Their implementation only differs in the stop condition and the final return value. JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu
5 participants
Their implementation only differs in the stop condition and the
final return value.
JerryScript-DCO-1.0-Signed-off-by: Mátyás Mustoha mmatyas@inf.u-szeged.hu