Move jerry-core API implementations and headers into dedicated subdirectories #1793

akosthekiss · 2017-04-28T13:57:20Z

Merged debugger-related public API declarations from
jerryscript-debugger.h to jerryscript.h. It's part of the
engine's public API, there is no need to force the users of the
library to include yet another header.

Moved all public API headers under the jerry-core/include
directory. This makes installing all the public headers easier.
Also, should we have new public headers in the future (not probable
but possible), their installation will be automatic, there will be
no need to update the build files. Moreover, this aligns better
with the structure of other libraries in the project (in those
cases, public headers always reside in <library>/include).

Moved all public API implementations under the jerry-core/api
directory. This cleans up the root directory of jerry-core,
moving all implementation code under "modules", i.e.,
subdirectories. This also makes the future splitting of the big and
monolithic jerry.c along features easier, if needed. (Debugger
and snapshot-related functions are already in separate sources.)

Note: jerry-snapshot.h is still under jerry-core/api, keeping
it as a non-public header.

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

akosthekiss · 2017-04-28T13:58:50Z

On top of the main commit, the PR contains an extra patch to speculatively adapt all targets to the include path change.

glistening · 2017-05-02T01:37:46Z

@akosthekiss I am thinking slightly differently about jerryscript-debugger.h. It is very closely related to jerry-core. However, it is the optional. For me, I don't want to have jerryscript-debugger related declarations when it is turned off. So I want to get your opinion about guarding jerryscript-debugger with JERRY_DEBUGGER like:

// In jerryscript.h
#ifdef JERRY_DEBUGGER
#include "jerryscript-debugger.h"
#endif

Or

// In jerryscript.h
#ifdef JERRY_DEBUGGER
// your merged part of jerryscript-debugger.h
#endif

LaszloLango · 2017-05-02T06:36:41Z

jerry-core/include/jerryscript.h

+bool jerry_debugger_is_connected (void);
+void jerry_debugger_stop (void);
+void jerry_debugger_continue (void);
+void jerry_debugger_stop_at_breakpoint (bool enable_stop_at_breakpoint);


I don't like this change. Why did you removed jerryscript-debugger.h?

zherczeg · 2017-05-02T06:46:59Z

I don't like merging debugger api to main api as well. Debugger is an extension, although it is so tightly coupled to the core it is not in a separate directory, but it is still an extension.

zherczeg · 2017-05-02T06:53:00Z

targets/esp8266/source/jerry_extapi.c

@@ -16,7 +16,7 @@
 #include <stdlib.h>
 #include <stdio.h>

-#include "jerry-core/jerryscript.h"
+#include "jerry-core/include/jerryscript.h"


This is so deep. What about creating a jerry-include top level directory and put the public api there.

This is "user error". The build system of ESP8266 should add "jerry-core/include" to the include path and then #include "jerryscript.h" only. Actually, even before this PR, the ESP8266 build system should have added "jerry-core" to the include path and never ever have anything like #include "jerry-core/xxxxx" in the code. But I did not want to go into rewriting the build systems of any of targets. (Where the build system was correctly written, no changes in the code was necessary -- see particle, riot-stm32f4, zephyr.)

zherczeg · 2017-05-02T06:59:02Z

Come to think of it, jerry debugger api could be an extension to jerry! Yes, the key functions are in the trunk but the whole concept of the debugger is an extension. Promise (#1796) could be an extension as well. @jiangzidong what do you think of that?

jiangzidong · 2017-05-02T07:04:11Z

I think Promise should be in the same position as string/number/boolean, because they are all JS builtin object. Do you mean to put every builtin-object related APIs into jerry-ext?

zherczeg · 2017-05-02T07:15:52Z

Good point. Ok, keep promise in top level.

akosthekiss · 2017-05-02T08:27:50Z

@glistening I've two arguments against conditionalizing the debugger-related functions:

jerryscript.h or jerryscript-debugger.h (whichever we chose) are public headers. At that point we should not rely on the build configuration of the library itself. When someone uses the library and includes the public headers, s/he may not/should not know what defines were used to build it.
Actually, those functions are really always implemented in the library. If the library was built without debugger support, their implementation is empty or dummy.

This is very much aligned with other optional features. E.g., all snapshot-related API functions work this way. They are always part of the API, but if the built library does not support snapshots, they don't do anything.

@zherczeg @LaszloLango My motivation for merging jerryscript-debugger.h into jerryscript.h was the above-mentioned analogy with snapshots. The debugger functions are always present API functions, only their implementation depends on the availability of a feature. That sounds very core to me. (Changing perspective: if debugger API function declarations should be in a separate header, should snapshot-related functions also be in a separate header?)

glistening · 2017-05-02T08:57:37Z

@akosthekiss I agree that not using the conditional guards in public header is better. Then, what about keep the jerryscript-debugger.h out of jerryscript.h ? Every developer who want debugger feature could turn on when s/he needs it. I prefer to turn off jerryscript debugger after debugging is done since it is likely to help to reduce the binary or runtime footprint. I think snapshot is slightly different case. Since it is positive to reduce the footprint ( am I right? ), I usually don't turn off ( I've never turned off snapshot). However, debugger is only useful when we debug.

glistening · 2017-05-02T10:16:41Z

since it is likely to help to reduce the binary or runtime footprint.

To myself, it is unlikely to help to reduce the binary or runtime memory usage by getting rid of jerryscript-debugger.h declarations from jerryscript.h when jerryscript debugger off.

@akosthekiss What do you mean by library in following ?

those functions are really always implemented in the library. If the library was built without debugger support, their implementation is empty or dummy.

Anyway, although I am personally not familiar with declarations in public header that have no definitions in library itself. However, if it is accepted convention, I will accept this convention.

zherczeg · 2017-05-02T10:50:43Z

@akosthekiss following that logic everything is core, since everything depends on the internals of JerryScript. Functions in jerryscript-debugger.h are debug specific functions, and there is no reason for them to be available in the main API.

akosthekiss · 2017-05-02T14:25:22Z

@glistening

What do you mean by library in following ?

those functions are really always implemented in the library. If the library was built without debugger support, their implementation is empty or dummy.

Sorry, that was ambiguous. What I meant was that jerry_debugger_is_connected and all the other jerry_debugger_xxx debugger-related API functions were always built into jerry-core (the library).

bool
jerry_debugger_is_connected (void)
{
#ifdef JERRY_DEBUGGER
  return JERRY_CONTEXT (debugger_flags) & JERRY_DEBUGGER_CONNECTED;
#else
  return false;
#endif /* JERRY_DEBUGGER */
} /* jerry_debugger_is_connected */

This is how jerry_debugger_is_connected looks now. The function is always in jerry-core. If it was built with debugger enabled, then it does something meaningful, but if not, its implementation is dummy.

So, no jerry-core API function is ever missing. Even if some features are disabled in a build, all API functions are callable -- but not necessarily do anything meaningful. This ensures that linking always succeeds. And it is possible to check during execution whether a feature is available or not (via jerry_is_feature_enabled).

glistening · 2017-05-02T14:48:27Z

@akosthekiss Thank you for clear explanation.

akosthekiss · 2017-05-02T15:00:00Z

@zherczeg

following that logic everything is core, since everything depends on the internals of JerryScript

Please, elaborate. I'm not sure I know what "everything" means here. Right now, the engine (the jerry-core library) has 3 public headers:

jerryscript.h: all the public API that embedders should/are allowed to call to interact with the engine,
jerryscript-port.h: all the function declarations that port providers must implement to get the engine work, and
jerryscript-debugger.h: function declarations needed to interact with the debugger API of the engine.

My point is, that I see no difference between jerryscript.h and jerryscript-debugger.h because both headers declare API of the engine. The debugger support is not just an extension of the engine, at least not in the sense of the jerry-ext library, because it is not possible to implement its functionality outside / on top of the engine.

That's why I've brought up the snapshot analogy: snapshot saving/execution is not a mandatory part of the engine, you can opt in or out; but it is impossible to implement snapshot saving/execution outside of the engine (outside of jerry-core).

IMHO:

argument handling, common external functions: Optional? Yes. Needed for ES compatibility? No. Needs accessing jerry-core internals? No. --> extension, implemented in jerry-ext, declared in jerryscript-ext/xxx.h
snapshots: Optional? Yes. Needed for ES compatibility? No. Needs accessing jerry-core internals? Yes. --> feature, implemented in jerry-core, declared in jerryscript.h
debugger support: like snapshots. So it's not an "extension" but a "feature". declared where?

glistening · 2017-05-02T23:02:45Z

@akosthekiss Here is my understanding: If it is impossible to implement some feature outside of jerry-core, it will be part of jerry-core even when it is optional feature. ( Hereafter I will call this feature core-extension to distinguish other extension (user-extension). in this comment. )

Here are my opinions on core-extensions

Where to keep the core-extension's declarations
You seem to like keep whole public things in jerryscript.h than having a separate header (i.e. jerryscript-debugger.h ) and including that header in jerryscript.h. Personally If there will be more core-extensions (currently - two : snapshot, debugger), I prefer to keep separate the feature's public header and include it in jerryscript.h and I found many open sources that use this approach if the public API grows up. Personally this separate container headers makes me easy to find the feature's api and maintain the code.
Zero overhead to keep the optional core-extension
In order to make the public header configuration agnostic, currently we provide dummy implementation in jerry-core library even when the option is off. It makes life easy and simple. However, I still hope zero overhead both in runtime performance and binary footprint since jerryscript is for low profile devices. And current approach put a few overhead for both. It may be removed by using macro like assert statement although the implementation may not look elegant and require tedious work.. What do you think?

akosthekiss · 2017-05-03T07:14:48Z

@glistening

Where to keep the core-extension's declarations

Actually, any approach can work with me if we apply that approach consistently. Where I disagreed with fellow reviewers was that right now the debugger API/header was treated differently than the rest, although I could not find the reason for that (analysis above).

There are several options to chose from:

keep the whole API in a single header (hard to break consistency then...) -- that's what I've chosen in the first version of this PR,
split the API into separate headers, one for the non-optional part, and one for each "feature" / "core-extension" -- that's the current status but not consistent (debugging feature has its own header, snapshots don't), and
split the API into separate headers, like option 2, but also include these headers into the main one -- I think that's your suggestion -- we don't do this even with the debugger header yet, but it's still a valid approach.

Any of the above works with me, just let's keep things consistent. Or convince me that we shouldn't ( @zherczeg @LaszloLango :) )

Note to self (and to all interested parties): option 2, when applied consistently (i.e., splitting out snapshot-related function declarations), is a breaking change, as library users will have to add an extra #include to their sources if they used snapshots before.

Zero overhead to keep the optional core-extension

I wouldn't plan function removal for this patch, although I agree that because of the goals of jerryscript, we should aim at zero overhead. Note 1: not including those functions with dummy implementation is a breaking change (users who could link before may not link afterwards). Note 2 (or a question): "removed by using macro like assert statement", I'll have to ask for some explanation this, I'm not sure how this would look like.

zherczeg · 2017-05-03T12:36:39Z

It is hard to judge the effect of choices. I would prefer the 1st option if the size of the header will not grow too large. But if it is, I would prefer the 3rd.

akosthekiss · 2017-05-03T15:57:00Z

I've added a commit that implements option 3 so that we can compare it against option 1. So, the question is, which variant to keep?

LaszloLango · 2017-05-04T07:26:09Z

@akosthekiss you convinced me. The first was better. :) I agree with @zherczeg

prefer the 1st option if the size of the header will not grow too large. But if it is, I would prefer the 3rd.

I think it isn't too big if we merge and it is easier for Jerry users to use only one header.

zherczeg · 2017-05-04T07:44:06Z

Another idea: what about making multiple headers, and jerryscript,h includes them all (it has no other code than including the other headers). So people need to include only one file, but we can separate type declarations, functions prototypes better.

LaszloLango · 2017-05-04T07:59:26Z

@zherczeg we may consider that too, but I think that could be done in a follow-up work.

zherczeg · 2017-05-04T08:02:26Z

It has already done in this patch.

LaszloLango · 2017-05-04T08:12:52Z

@zherczeg sure. My bad. Too many open PRs :)

akosthekiss · 2017-05-04T08:35:23Z

@zherczeg Sorry, I'm a bit confused now. The current status of this PR (all 3 commits together) implement option 3 (see above; non-optional jerry-core API in jerryscript.h, feature-dependent APIs in jerryscript-FEATURE.h). But two recent comments seem to conflict with this and with each other: "what about making multiple headers, and jerryscript,h includes them all (it has no other code than including the other headers)" vs "It has already done in this patch".

Note (for option 3, and similar variants): if feature APIs are split out into separate headers which are included by jerryscript.h, then either they cannot be used directly (i.e., user must not include jerryscript-snapshot.h directly) or these feature headers must include jerryscript.h, too (because API function declarations may need types defined in jerryscript.h; that's how jerryscript-snapshot.h looks right now; works, but it's not nice, IMHO).

zherczeg · 2017-05-04T08:43:31Z

Yes, users can only include jerryscript.h. The current API would be moved to jerryscript-api.h, which would be the first file in the include list of jerryscript.h. The rest is already in the patch, so this is little work.

akosthekiss · 2017-05-04T09:23:25Z

I've updated the PR according to the latest comments (added a 4th commit that splits out the main API into a new header).

zherczeg

LGTM

zherczeg · 2017-05-04T10:12:24Z

jerry-core/include/jerryscript.h

+
+#include "jerryscript-api.h"
+#include "jerryscript-snapshot.h"
+#include "jerryscript-debugger.h"


Btw should be in alphabetic order.

LaszloLango · 2017-05-04T10:36:41Z

@akosthekiss @zherczeg, jerryscript-api.h is quite odd. jerryscript-debugger.h and jerryscript-snapshot.h are also api headers. What do you think about jerryscript-common.h or jerryscript-core.h?

akosthekiss · 2017-05-04T10:41:46Z

@LaszloLango both -api.h and -core.h are OK with me. (-common.h looks a bit strange to me)

zherczeg · 2017-05-04T10:47:24Z

I like the -core postfix. or -base.

…ectories Moved all public API headers under the `jerry-core/include` directory. This makes installing all the public headers easier. Also, should we have new public headers in the future, their installation will be automatic, there will be no need to update the build files. Moreover, this aligns better with the structure of other libraries in the project (in those cases, public headers always reside in `<library>/include`). Moved all public API implementations under the `jerry-core/api` directory. This cleans up the root directory of `jerry-core`, moving all implementation code under "modules", i.e., subdirectories. This also makes the future splitting of the big and monolithic `jerry.c` along features easier, if needed. (Debugger and snapshot-related functions are already in separate sources.) Notes: * `jerryscript.h` is split up to separate header files along feature boundaries. These new headers are included by `jerryscript.h`, so this is not a breaking change but header modularization only. * `jerry-snapshot.h` is still under `jerry-core/api`, keeping it as a non-public header. * This commit also adapts all targets to the include path change. JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

akosthekiss · 2017-05-04T13:04:53Z

I've changed jerryscript-api.h to -core.h and squashed all commits.

LaszloLango

LGTM

akosthekiss added the api Related to the public API label Apr 28, 2017

LaszloLango reviewed May 2, 2017

View reviewed changes

zherczeg reviewed May 2, 2017

View reviewed changes

akosthekiss mentioned this pull request May 2, 2017

Common but non-standard JS function implementations in jerry-ext #1787

Merged

akosthekiss force-pushed the api-refactor branch from c531f9f to 1e1012c Compare May 3, 2017 09:53

zherczeg mentioned this pull request May 3, 2017

Support multiple primary functions in a single snapshot. #1797

Merged

akosthekiss force-pushed the api-refactor branch from e5d4560 to 47be8c7 Compare May 4, 2017 09:19

zherczeg approved these changes May 4, 2017

View reviewed changes

zherczeg reviewed May 4, 2017

View reviewed changes

akosthekiss force-pushed the api-refactor branch from 47be8c7 to d24cf53 Compare May 4, 2017 10:32

akosthekiss force-pushed the api-refactor branch from d24cf53 to a910791 Compare May 4, 2017 13:00

LaszloLango approved these changes May 4, 2017

View reviewed changes

akosthekiss merged commit 91e976f into jerryscript-project:master May 4, 2017

akosthekiss deleted the api-refactor branch May 4, 2017 13:28

Move jerry-core API implementations and headers into dedicated subdirectories #1793

Move jerry-core API implementations and headers into dedicated subdirectories #1793

Uh oh!

Conversation

akosthekiss commented Apr 28, 2017

Uh oh!

akosthekiss commented Apr 28, 2017

Uh oh!

glistening commented May 2, 2017

Uh oh!

LaszloLango May 2, 2017

Choose a reason for hiding this comment

Uh oh!

zherczeg commented May 2, 2017

Uh oh!

zherczeg May 2, 2017

Choose a reason for hiding this comment

Uh oh!

akosthekiss May 2, 2017

Choose a reason for hiding this comment

Uh oh!

zherczeg commented May 2, 2017

Uh oh!

jiangzidong commented May 2, 2017

Uh oh!

zherczeg commented May 2, 2017

Uh oh!

akosthekiss commented May 2, 2017

Uh oh!

glistening commented May 2, 2017

Uh oh!

glistening commented May 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg commented May 2, 2017

Uh oh!

akosthekiss commented May 2, 2017

Uh oh!

glistening commented May 2, 2017

Uh oh!

akosthekiss commented May 2, 2017

Uh oh!

glistening commented May 2, 2017

Uh oh!

akosthekiss commented May 3, 2017

Uh oh!

zherczeg commented May 3, 2017

Uh oh!

akosthekiss commented May 3, 2017

Uh oh!

LaszloLango commented May 4, 2017

Uh oh!

zherczeg commented May 4, 2017

Uh oh!

LaszloLango commented May 4, 2017

Uh oh!

zherczeg commented May 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LaszloLango commented May 4, 2017

Uh oh!

akosthekiss commented May 4, 2017

Uh oh!

zherczeg commented May 4, 2017

Uh oh!

akosthekiss commented May 4, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg May 4, 2017

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented May 4, 2017

Uh oh!

akosthekiss commented May 4, 2017

Uh oh!

zherczeg commented May 4, 2017

Uh oh!

akosthekiss commented May 4, 2017

glistening commented May 2, 2017 •

edited

Loading

zherczeg commented May 4, 2017 •

edited

Loading