-
Notifications
You must be signed in to change notification settings - Fork 685
Implement function.toString operation #4752
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
Conversation
| OPTIONS_PROFILE_MIN = ['--profile=minimal'] | ||
| OPTIONS_PROFILE_ES51 = ['--profile=es5.1'] | ||
| OPTIONS_PROFILE_ESNEXT = ['--profile=es.next'] | ||
| OPTIONS_PROFILE_ESNEXT = ['--profile=es.next', '--function-to-string=on'] |
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.
Let me know if there is a better place for this config.
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.
Function.prototype.toString changes are part of ES2019 spec. Why don't we simple enable it for ES next? Of course we could have an option to be able to disable it if the user don't want to use it.
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.
This "feature" can be costly from memory use perspective, which is quite the opposite of the aims of this engine. However, if somebody needs this, it can be enabled.
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.
OK, see.
tests/test262-esnext-excludelist.xml
Outdated
| <test id="built-ins/Function/prototype/toString/bound-function.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/built-in-function-object.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-complex-heritage.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-implicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-expression-explicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-expression-implicit-ctor.js"><reason></reason></test> |
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.
There are many still failing tests in Function/prototype/toString. Do we plan to support them too in a follow-up PR or in this PR in next iterations?
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.
Those are mostly the "native" and "whole class" variants. We can do them incrementally later. The latter is not exactly trivial.
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.
OK, sounds good.
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.
Turned out the "native" part is quite trivial, so added it to this patch. The "class" part still remains.
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 see many JERRY_ESNEXT || JERRY_FUNCTION_TO_STRING ifdef guards. This new feature is part of ES2019 spec, so I wouldn't let the user to enable it in case of !JERRY_ESNEXT. I would simple make JERRY_FUNCTION_TO_STRING depends on JERRY_ESNEXT and we should force it in config.h near "JERRY_ESNEXT should be enabled too to enable JERRY_BUILTIN_xxxxx macro." error message.
One more thing. If it is disabled by default but enabled always during testing in case of ESNEXT would cause build failures. Can we test !JERRY_FUNCTION_TO_STRING build configuration by buildoption tests?
| #if JERRY_FUNCTION_TO_STRING \ | ||
| || !(JERRY_FUNCTION_TO_STRING) | ||
| LIT_MAGIC_STRING_DEF (LIT_MAGIC_STRING__FUNCTION_TO_STRING, "function(){/* ecmascript */}") | ||
| #endif |
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.
x || !x is always true. Do we know why this strange conditions are generated?
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.
#4751 aims to fix that
tests/test262-esnext-excludelist.xml
Outdated
| <test id="built-ins/Function/prototype/toString/bound-function.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/built-in-function-object.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-complex-heritage.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-explicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-declaration-implicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-expression-explicit-ctor.js"><reason></reason></test> | ||
| <test id="built-ins/Function/prototype/toString/class-expression-implicit-ctor.js"><reason></reason></test> |
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.
OK, sounds good.
| OPTIONS_PROFILE_MIN = ['--profile=minimal'] | ||
| OPTIONS_PROFILE_ES51 = ['--profile=es5.1'] | ||
| OPTIONS_PROFILE_ESNEXT = ['--profile=es.next'] | ||
| OPTIONS_PROFILE_ESNEXT = ['--profile=es.next', '--function-to-string=on'] |
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.
OK, see.
93c2e21 to
df1f70e
Compare
galpeter
left a comment
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.
lgtm
May increase the memory consumtpion heavily. JerryScript-DCO-1.0-Signed-off-by: Zoltan Herczeg zherczeg.u-szeged@partner.samsung.com
May increase the memory consumption heavily.