Skip to content

Conversation

@akosthekiss
Copy link
Member

There was quite some confusion about terminology around instances
and contexts. All the docs mentioned external contexts but
functions and types were referring to instances, and the relation
between these two concepts were not clear. This commit keeps
(external) context as the only surviving concept.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

There was quite some confusion about terminology around instances
and contexts. All the docs mentioned external contexts but
functions and types were referring to instances, and the relation
between these two concepts were not clear. This commit keeps
(external) context as the only surviving concept.

JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]
@akosthekiss akosthekiss added ecma core Related to core ECMA functionality api Related to the public API labels Aug 31, 2018
@akosthekiss
Copy link
Member Author

Part of the context rework PR series, follow-up to #2500 .

@zherczeg zherczeg mentioned this pull request Sep 3, 2018
/* The value of external context members must be preserved across initializations and cleanups. */
#ifdef JERRY_ENABLE_EXTERNAL_CONTEXT
#ifndef JERRY_SYSTEM_ALLOCATOR
jmem_heap_t *heap_p; /**< point to the heap aligned to JMEM_ALIGNMENT. */
Copy link
Member

Choose a reason for hiding this comment

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

I would put this before or after the context, probably after would be better, since the starting offset would be fixed. Then we don't need this pointer.

Copy link
Member Author

Choose a reason for hiding this comment

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

The instance-based approach also contained this pointer, at the end of the instance struct, even if the heap always started right after the instance. I'd keep this pointer as it allows for future developments, e.g., one that allows placing context structures and heaps in different memory regions (which is already available for the builtin global context approach but is not available for external contexts right now).

* all global variables for Jerry
*/
typedef struct
struct jerry_context_t
Copy link
Member

Choose a reason for hiding this comment

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

Why do you remove the typedef?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is typedef'd in jerryscript-core.h. A typedef cannot be declared twice. jerry_context_t is taking over the place of jerry_instance_t, so it has to be opaquely forward declared in the public API header. But that means that only the struct can be declared here.

Note: jerry_instance_t was handled exactly the same way.

@akosthekiss
Copy link
Member Author

@zherczeg Thanks for the review, your comments have been answered in-line. Please, let me know if you have further change requests (or still maintain the current ones).

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

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

@akosthekiss akosthekiss merged commit 30b7a72 into jerryscript-project:master Sep 4, 2018
@akosthekiss akosthekiss deleted the context-rework-instance branch September 4, 2018 11:56
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 ecma core Related to core ECMA functionality

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants