Skip to content

Added the _restore function to "unpop" the argument stack. #2592

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

t-harvey
Copy link

This is a pull request based on the discussion #2581.

Thanks!

@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch 6 times, most recently from ad36072 to 147d9e2 Compare November 12, 2018 21:40
@zherczeg
Copy link
Member

For me this ok, although I haven't reviewed the original code. Perhaps @akosthekiss could help.

@LaszloLango
Copy link
Contributor

Please add some test into the tests/unit-ext/test-ext-arg.c file.

@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch 4 times, most recently from 506f13c to f26716b Compare November 16, 2018 21:15
@zherczeg
Copy link
Member

zherczeg commented Dec 5, 2018

I think another round of review could be done here.

@LaszloLango
Copy link
Contributor

Sorry for the late response. I am not sure this is right way to do this, but I am not against this PR. Please add the new API function to the related documentation (docs/09.EXT-REFERENCE-ARG.md). The patch is good to me after that.

@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch 4 times, most recently from 6a516ea to a9d05e3 Compare December 5, 2018 22:48
Copy link
Contributor

@LaszloLango LaszloLango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code LGTM, but please squash the commits into one.

@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch from a9d05e3 to b4a87c8 Compare December 7, 2018 22:02
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me as well, since there was no objection. Only some style issues needed to fix.

@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch from b4a87c8 to 67b49ae Compare December 14, 2018 22:33
Added a unit test for the jerryx_arg_js_iterator_restore function.
Added the specification to the documentation.

JerryScript-DCO-1.0-Signed-off-by: Timothy Harvey [email protected]
@t-harvey t-harvey force-pushed the add_restore_function_to_jerry_ext_helpers branch from 67b49ae to 31d2156 Compare December 14, 2018 23:37
Copy link
Member

@zherczeg zherczeg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zherczeg zherczeg merged commit cfdb5ed into jerryscript-project:master Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants