Skip to content

Allow the JS objects to have more than one native pointer data #2814

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 16, 2019
Merged

Allow the JS objects to have more than one native pointer data #2814

merged 1 commit into from
Apr 16, 2019

Conversation

rerobika
Copy link
Member

@rerobika rerobika commented Apr 8, 2019

Currently JS objects can only have one native pointer data which could be a limitation in special cases. This patch allows to register multiple native infos, which can be accessed/associated with the corresponding jerry_object_native_info_t.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]

@rerobika
Copy link
Member Author

rerobika commented Apr 8, 2019

Note: The patch contains API breaking changes so if the reviewers support this idea this should be land before the 2.0 release. I highly recommend to check this example to understand the motivation of the PR.

zherczeg
zherczeg approved these changes Apr 9, 2019
View reviewed changes
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.

Looks like a nice change.

jerry-core/ecma/base/ecma-globals.h
{
void *data_p; /**< points to the data of the object */
ecma_object_native_info_t *info_p; /**< native info */
struct ecma_native_pointer_t *next_p; /**< points to the next ecma_native_pointer_t element */
Copy link
Member

@zherczeg zherczeg Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This could be changed later but adding a pointer effectively doubles the size of this structure. (Jerry allocates memory in 8 byte blocks)

@LaszloLango
Copy link
Contributor

I like this proposal. I have two comments on this:

  1. a jerry_delete_object_native_pointer function is also needed
  2. jerry_get_object_native_pointer should return with void * instead of bool, where NULL means there is no such pointer.

@legendecas
Copy link
Contributor

  1. jerry_get_object_native_pointer should return with void * instead of bool, where NULL means there is no such pointer.

There might be cases that the native pointer might be NULL yet the JS objects been bound with gc-callbacks.

@rerobika
Copy link
Member Author

rerobika commented Apr 9, 2019

There might be cases that the native pointer might be NULL yet the JS objects been bound with gc-callbacks.

@legendecas Is this a frequently used structure? Could you show me an example for it?

@LaszloLango
Copy link
Contributor

@rerobika @legendecas I don't mind to keep the NULL value as a valid native pointer.

@legendecas
Copy link
Contributor

Is this a frequently used structure? Could you show me an example for it?

I don't see the frequent usage of this behavior nor any example for it, sorry for that. Yet if the API doesn't force the native pointer to be non-NULL, then that is a valid usage anyway.

@rerobika rerobika force-pushed the native_pointers branch 2 times, most recently from dc065fe to a3cdc7b Compare April 11, 2019 15:35
@LaszloLango LaszloLango changed the title [Proposal] Allow the JS objects to have more than one native pointer data Allow the JS objects to have more than one native pointer data Apr 12, 2019
LaszloLango
LaszloLango requested changes Apr 15, 2019
View reviewed changes
@rerobika
Allow the JS objects to have more than one native pointer data
43b44a7 
Currently JS objects can only have one native pointer data which could be a limitation in special cases.
This patch allows to register multiple native infos, which can be accessed/associated with the corresponding `jerry_object_native_info_t`.

JerryScript-DCO-1.0-Signed-off-by: Robert Fancsik [email protected]
@rerobika
Copy link
Member Author

@LaszloLango Thanks for the review, I updated the PR.

LaszloLango
LaszloLango approved these changes Apr 16, 2019
View reviewed changes
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.

LGTM

@LaszloLango LaszloLango added this to the Release 2.0 milestone Apr 16, 2019
@LaszloLango LaszloLango added api Related to the public API development Feature implementation labels Apr 16, 2019
@LaszloLango LaszloLango merged commit b3f4aa6 into jerryscript-project:master Apr 16, 2019
@rerobika rerobika deleted the native_pointers branch June 16, 2020 08:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zherczeg zherczeg zherczeg approved these changes

@LaszloLango LaszloLango LaszloLango approved these changes

Assignees
No one assigned
Labels
api Related to the public API development Feature implementation
Projects
None yet
Milestone
Release 2.0
Development

Successfully merging this pull request may close these issues.
4 participants
@rerobika @LaszloLango @legendecas @zherczeg