Implement Array.prototype.toString #36
Conversation
galpeter
force-pushed
the
array_prototype_tostring
branch
4 times, most recently
from
54d3a42 to
a4fd8cd
Compare
|
@egavrin, @ruben-ayrapetyan can you review please? We need this for testing RegExp result. |
| */ | ||
|
|
||
| ecma_completion_value_t | ||
| ecma_op_object_prototype_tostring (const ecma_value_t this_arg) /**< this argument */ |
There was a problem hiding this comment.
@galpeter ,
please move the module to jerry-core/ecma/builtin-objects component, as it is common part of several builtin's implementation.
You could name it as the following: 'ecma-builtiin-helpers.h' and 'ecma-builtin-helpers.cpp'.
Please, rename ecma_op_object_prototype_tostring to ecma_builtin_helper_object_to_string,
and reflect the fact that it is common part of several built-in routines in the comment with list of the several built-in routines.
There was a problem hiding this comment.
@ruben-ayrapetyan , so you mean I should list in the comment where the method is used? Currently I know only two of the use cases, but that could easily change in the future. Is it really needed to list all usage occurrences in the comments? (Or did I misunderstood something?)
There was a problem hiding this comment.
@galpeter , yes, I mean comment with list of call sites (names of calling functions).
Currently I know only two of the use cases, but that could easily change in the future
How would this occur? Are there any other ECMA-defined call sites of the algorithm?
Anyway, for me it seems better to list all usage occurrences for the function if they would somehow occur.
There was a problem hiding this comment.
Currently I know only two of the use cases, but that could easily change in the future
How would this occur? Are there any other ECMA-defined call sites of the algorithm?
Anyway, for me it seems better to list all usage occurrences for the function if they would somehow occur.
Simple way: I did not read the whole ECMA standard to see where it is used, as I didn't think that it is needed to have such information.
Question: Do we plan to have every function's commit contain the list of calling functions? What is the benefit?
There was a problem hiding this comment.
- list for the function should be in the comment to the function, not in the commit's comment, as you've written (maybe by mistake?)
- no, this is specific case - built-in implementation helper that is significant part of a built-in routine's algorithm. it is better to describe why it was extracted, and one of the way do this - to list call sites.
- I hope, that the case of extracting built-in routines' parts would be very rare
- you can see that the practice to reference a function in comment to another function is already used in some engine parts where we consider it is useful and prevents misunderstanding.
So, not, we don't plan to have every function's comment to contain the list of calling functions,
but plan to have them for some specific cases, to improve code readability.
Update:
"3. I hope, that the case of extracting built-in routines' parts would be very rare" - I mean case of reusing the parts between implementation of different built-in objects.
JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]
galpeter
force-pushed
the
array_prototype_tostring
branch
from
a4fd8cd to
c16e065
Compare
|
@ruben-ayrapetyan, I've updated the pull request based on your suggestions. |
|
@galpeter |
|
LGTM |
…iments-dev Experiments dev
JerryScript-DCO-1.0-Signed-off-by: Peter Gal [email protected]