Skip to content
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

Use 'ecma_value_t' in Jerry API #1138

Merged

Conversation

LaszloLango
Copy link
Contributor

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]

@LaszloLango LaszloLango added enhancement An improvement api Related to the public API labels Jun 13, 2016
@LaszloLango LaszloLango added this to the Release v1.0 milestone Jun 13, 2016

/**
* Converters of 'jerry_value_t'
*/
jerry_string_t *jerry_value_to_string (const jerry_value_t *);
jerry_value_t jerry_value_to_string (const jerry_value_t value);
Copy link
Member

Choose a reason for hiding this comment

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

"value" is not needed.

@zherczeg
Copy link
Member

I think the general direction is very good. However, I feel people will not understand how the exception handling works in Jerry. Namely a value has an exception bit, and a value with an exception bit set cannot be passed to any function as input argument.

I am open to other ideas, but I see two directions at this moment:

  • all functions should return with a bool (exception or not), and a value as an output argument. The output contains the exception value in case of exception, but the exception bit is cleared, so they can pass it to other functions.
  • all functions return with a jerry value, where the exception bit can be set. There is an API function to check exceptions. For all input arguments the exception bit is cleared manually (so people cannot accidentally pass exception objects as values when setting a field of an object).

@LaszloLango
Copy link
Contributor Author

@zherczeg, I prefer the second, but maybe the first one would be better for newcomers.

@zherczeg
Copy link
Member

The second is obviously simpler and faster, but it might be confusing. What do others think?

@dbatyai
Copy link
Member

dbatyai commented Jun 14, 2016

In my opinion going with the simpler solution is always the best choice, and would also behave similarly to how the engine currently works. The first approach would be the complete opposite, which, the way I see it, could create more confusion than the second one.

@LaszloLango
Copy link
Contributor Author

@zherczeg, I raised an issue for this problem. I think we can do it in a follow up patch, because this PR is big enough and barely related to the problem.

JerryScript-DCO-1.0-Signed-off-by: László Langó [email protected]
@zherczeg
Copy link
Member

Ok, LGTM

@dbatyai
Copy link
Member

dbatyai commented Jun 14, 2016

LGTM

@LaszloLango LaszloLango merged commit a816ab8 into jerryscript-project:master Jun 14, 2016
@LaszloLango LaszloLango mentioned this pull request Jun 15, 2016
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Related to the public API enhancement An improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants