Support a user context void * pointer in jerry_context_t (#1717) #1727

gabrielschulhof · 2017-04-11T12:03:34Z

This modification makes it possible to initialize a context in such a
way that a void * pointer is stored inside the context and is made
available via a new jerry_get_user_context() API.

The pointer is initialized via a new jerry_init_with_user_context()
API, which calls the existing jerry_init(), after which it sets the
value of the new user_context element in the jerry_context_t
structure using the context allocation callback provided as the second
parameter to the new jerry_init_with_user_context() API. The location
of the cleanup function responsible for deallocating the pointer created
by the context allocation callback is provided as the third parameter.
This location is stored in the context along with the pointer itself.

When a context is discarded via jerry_cleanup(), the user context
cleanup function is called to dispose of the pointer stored within the
context.

The semantics behind the API are such that it is now possible to choose
for each context an agent which manages arbitrary user data keyed to the
given context. The agent must be chosen at context instantiation time
and cannot be changed afterwards, remaining in effect for the lifetime
of the context.

Fixes #1717

JerryScript-DCO-1.0-Signed-off-by: Gabriel Schulhof [email protected]

martijnthe · 2017-04-11T12:08:30Z

Nice 💯

zherczeg

Please also add a documentation in docs/02.API-REFERENCE.md

zherczeg · 2017-04-11T12:07:30Z

jerry-core/jcontext/jcontext.h

@@ -103,6 +103,8 @@ typedef struct
  uint8_t valgrind_freya_mempool_request; /**< Tells whether a pool manager
                                           *   allocator request is in progress */
 #endif /* JERRY_VALGRIND_FREYA */
+  void *user_context;
+  jerry_user_context_deinit_cb user_context_deinit_cb;


These are mandatory pointers, I would put them after vm_frame_ctx_t *vm_top_context_p;

zherczeg · 2017-04-11T12:08:34Z

jerry-core/jerry.c

+ */
+void
+jerry_init_with_user_context (jerry_init_flag_t flags, jerry_user_context_init_cb init_cb,
+                              jerry_user_context_deinit_cb deinit_cb)


Full stop after each sentence. Please add a comment for each function. Coding style is in #1459.

zherczeg · 2017-04-11T12:09:48Z

jerry-core/jerry.c

 } /* jerry_cleanup */

 /**
+ * Retrieve user context


Please add return value description.

zherczeg · 2017-04-11T12:11:11Z

tests/unit/test-user-context.c

+static bool user_context_new_was_called = false;
+static bool user_context_free_was_called = false;
+
+static void *user_context_new (void)


Newline after static void.

gabrielschulhof · 2017-04-12T08:04:59Z

@zherczeg I have addressed your review comments. Could you please take another look?

LaszloLango · 2017-04-12T09:19:34Z

docs/02.API-REFERENCE.md

+```c
+void
+jerry_init_with_user_context (jerry_init_flag_t flags, jerry_user_context_init_cb init_cb,
+                              jerry_user_context_deinit_cb deinit_cb);


missing line break:

jerry_init_with_user_context (jerry_init_flag_t flags, jerry_user_context_init_cb init_cb, jerry_user_context_deinit_cb deinit_cb);

LaszloLango · 2017-04-12T09:20:40Z

docs/02.API-REFERENCE.md

+
+```c
+void *
+init_user_context(void) {


init_user_context (void) {

LaszloLango · 2017-04-12T09:21:07Z

docs/02.API-REFERENCE.md

+} /* init_user_context */
+
+void
+free_user_context(void *context) {


free_user_context (void *context) {

LaszloLango · 2017-04-12T09:21:49Z

docs/02.API-REFERENCE.md

+
+} /* free_user_context */
+{
+


move this empty line before the open brace

LaszloLango · 2017-04-12T09:22:19Z

docs/02.API-REFERENCE.md

+} /* free_user_context */
+{
+
+  /* init_user_context() will be called before the call below returns */


missing space: init_user_context ()

LaszloLango · 2017-04-12T09:22:38Z

docs/02.API-REFERENCE.md

+
+  /* init_user_context() will be called before the call below returns */
+  jerry_init_with_user_context (JERRY_INIT_SHOW_OPCODES | JERRY_INIT_SHOW_REGEXP_OPCODES,
+    init_user_context, free_user_context);


wrong indentation

LaszloLango · 2017-04-12T09:23:07Z

docs/02.API-REFERENCE.md

+
+  /* ... */
+
+  /* free_user_context() will be called before the call below returns */


missing space: free_user_context ()

LaszloLango · 2017-04-12T09:23:27Z

docs/02.API-REFERENCE.md

+jerry_get_user_context (void);
+```
+
+- return value: the pointer that was assigned during `jerry_init_with_user_context()`


missing space before (

LaszloLango · 2017-04-12T09:24:11Z

jerry-core/jerry.c

+ * Initialize Jerry engine with custom user context.
+ */
+void
+jerry_init_with_user_context (jerry_init_flag_t flags, jerry_user_context_init_cb init_cb,


missing line break

LaszloLango · 2017-04-12T09:24:49Z

jerry-core/jerryscript.h

@@ -189,11 +199,14 @@ typedef struct
 * General engine functions
 */
 void jerry_init (jerry_init_flag_t flags);
+void jerry_init_with_user_context (jerry_init_flag_t flags, jerry_user_context_init_cb init_cb,


gabrielschulhof · 2017-04-12T11:02:12Z

@zherczeg @LaszloLango I have now address all review comments so far.

zherczeg

Please fix the _p and squash commits. LGTM after that.

zherczeg · 2017-04-12T12:31:20Z

jerry-core/jcontext/jcontext.h

@@ -61,6 +61,8 @@ typedef struct
  ecma_lit_storage_item_t *number_list_first_p; /**< first item of the literal number list */
  ecma_object_t *ecma_global_lex_env_p; /**< global lexical environment */
  vm_frame_ctx_t *vm_top_context_p; /**< top (current) interpreter context */
+  void *user_context; /**< user-provided context-specific pointer */


user_context_p - this is a pointer

gabrielschulhof · 2017-04-12T18:57:34Z

@LaszloLango should all be good now. Please take another look!

jiangzidong · 2017-04-13T01:08:00Z

docs/02.API-REFERENCE.md

+function calls the callback given in its `init_cb` parameter to allocate the memory for the pointer
+and it stores the function pointer given in the `deinit_cb` parameter along with the pointer so that
+it may be called to free the stored pointer when the context is discarded.
+


Could you please describe the relationship among jerry_init_with_user_context, jerry_init and jerry_cleanup.
e.g.

in the ##jerry_init: Note: If a user context need to be initialized, please use jerry_init_with_user_context instead.

in the ##jerry_init_with_user_context: Note: jerry_init will be called inside jerry_init_with_user_context, and deinit_cb will be called in the jerry_cleanup

in the ##jerry_cleanup: If the engine is Initialized by jerry_init_with_user_context with deinit_cb, then the deinit_cb will be called during cleanup.

What do you think?

This is a good idea.

LaszloLango · 2017-04-13T07:10:35Z

docs/02.API-REFERENCE.md

+{
+  /* init_user_context () will be called before the call below returns */
+  jerry_init_with_user_context (JERRY_INIT_SHOW_OPCODES | JERRY_INIT_SHOW_REGEXP_OPCODES,
+                                init_user_context, free_user_context);


missing line break. Every parameter should be on separate line.

LaszloLango · 2017-04-13T07:11:39Z

jerry-core/jerry.c

+void
+jerry_init_with_user_context (jerry_init_flag_t flags,
+                              jerry_user_context_init_cb init_cb,
+                              jerry_user_context_deinit_cb deinit_cb)


Missing comments of the arguments.

gabrielschulhof · 2017-04-13T12:28:46Z

@jiangzidong, @LaszloLango how's about now?

…-project#1717) This modification makes it possible to initialize a context in such a way that a `void *` pointer is stored inside the context and is made available via a new `jerry_get_user_context()` API. The pointer is initialized via a new `jerry_init_with_user_context()` API, which calls the existing `jerry_init()`, after which it sets the value of the new `user_context` element in the `jerry_context_t` structure using the context allocation callback provided as the second parameter to the new `jerry_init_with_user_context()` API. The location of the cleanup function responsible for deallocating the pointer created by the context allocation callback is provided as the third parameter. This location is stored in the context along with the pointer itself. When a context is discarded via `jerry_cleanup()`, the user context cleanup function is called to dispose of the pointer stored within the context. The semantics behind the API are such that it is now possible to choose for each context an agent which manages arbitrary user data keyed to the given context. The agent must be chosen at context instantiation time and cannot be changed afterwards, remaining in effect for the lifetime of the context. Fixes jerryscript-project#1717 JerryScript-DCO-1.0-Signed-off-by: Gabriel Schulhof [email protected]

zherczeg · 2017-04-13T12:31:33Z

Still LGTM.

LaszloLango

Thanks for the update. LGTM

jiangzidong

Thanks. LGTM

zherczeg requested changes Apr 11, 2017

View reviewed changes

gabrielschulhof force-pushed the 1717-user-context-support branch 3 times, most recently from be4afe0 to afed69f Compare April 11, 2017 20:30

LaszloLango requested changes Apr 12, 2017

View reviewed changes

zherczeg approved these changes Apr 12, 2017

View reviewed changes

gabrielschulhof force-pushed the 1717-user-context-support branch 2 times, most recently from 787dbf7 to d6d3dea Compare April 12, 2017 12:48

jiangzidong reviewed Apr 13, 2017

View reviewed changes

LaszloLango requested changes Apr 13, 2017

View reviewed changes

gabrielschulhof force-pushed the 1717-user-context-support branch from d6d3dea to f34219c Compare April 13, 2017 12:28

gabrielschulhof force-pushed the 1717-user-context-support branch from f34219c to 069d5cd Compare April 13, 2017 12:29

LaszloLango approved these changes Apr 13, 2017

View reviewed changes

jiangzidong approved these changes Apr 14, 2017

View reviewed changes

jiangzidong merged commit b4a3985 into jerryscript-project:master Apr 14, 2017

gabrielschulhof deleted the 1717-user-context-support branch April 18, 2017 21:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support a user context void * pointer in jerry_context_t (#1717) #1727

Support a user context void * pointer in jerry_context_t (#1717) #1727

gabrielschulhof commented Apr 11, 2017

martijnthe commented Apr 11, 2017

zherczeg left a comment

zherczeg Apr 11, 2017

zherczeg Apr 11, 2017

zherczeg Apr 11, 2017

zherczeg Apr 11, 2017

gabrielschulhof commented Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

LaszloLango Apr 12, 2017

gabrielschulhof commented Apr 12, 2017

zherczeg left a comment

zherczeg Apr 12, 2017

gabrielschulhof commented Apr 12, 2017

jiangzidong Apr 13, 2017 •

edited

Loading

zherczeg Apr 13, 2017

LaszloLango Apr 13, 2017

LaszloLango Apr 13, 2017

gabrielschulhof commented Apr 13, 2017

zherczeg commented Apr 13, 2017

LaszloLango left a comment

jiangzidong left a comment


		/* ... */

		/* free_user_context() will be called before the call below returns */

Support a user context void * pointer in jerry_context_t (#1717) #1727

Support a user context void * pointer in jerry_context_t (#1717) #1727

Conversation

gabrielschulhof commented Apr 11, 2017

martijnthe commented Apr 11, 2017

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 12, 2017

zherczeg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 12, 2017

jiangzidong Apr 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielschulhof commented Apr 13, 2017

zherczeg commented Apr 13, 2017

LaszloLango left a comment

Choose a reason for hiding this comment

jiangzidong left a comment

Choose a reason for hiding this comment

jiangzidong Apr 13, 2017 •

edited

Loading