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

akosthekiss · 2017-04-26T18:39:35Z

This PR splits out the implementation of "common but non-standard" functions
from PR #1749. The goal is to save various targets from re-implementing external
function handlers for print, gc, or assert. The idea is to provide these
handlers as a module of the now-forming jerry-ext library.

The PR is still Work in Progress, because

there is a debate whether all function handlers should be in one module (i.e.,
source sub-directory) of jerry-ext, or each should go into its own subdir,
there is no consensus what would be a good name for the module(s),
I'm not sure how print could be implemented in the most portable but also
efficient way (because a reusable implementation cannot rely on libc's
printf -- but then each port/app must somehow provide its own C-level
printing routine, which we are just getting rid of in Remove the built-in print function #1749...), and
jerry-ext is not in master yet, so there is no build system, CMakeLists,
etc. in this PR.

Note: we can also conclude that such an approach as presented here actually does not add much to reusability, because ports will not share these implementations, e.g., because they have seriously different needs for assert or print. Please, give your feedback, maintainers of targets are especially summoned.

zherczeg · 2017-05-02T06:26:25Z

From all extensions, this should land first because it is simple, and we can define the extension format here.

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

jerry-ext/extfunc/extfunc-assert.c

+ * limitations under the License.
+ */
+
+#include "jerryscript-ext/extfunc.h"


I don't remember what was the conclusion, shall we use ext or util?

I just realized that ext can mean extension or extra, and here it sounds like extra.

The 'ext' in 'jerryscript-ext' stands for 'extension' (and there, we've concluded to use 'ext', not 'util'). The 'ext' in 'extfunc' stands for 'external', as in 'jerry_create_external_function' (and there, we haven't concluded what would be the best name -- I've copied this over from the original PR as I had to give some name to the jerry-ext module).

zherczeg · 2017-05-02T06:28:37Z

jerry-ext/extfunc/extfunc-assert.c

+ * limitations under the License.
+ */
+
+#include "jerryscript-ext/extfunc.h"


I just realized that ext can mean extension or extra, and here it sounds like extra.

zherczeg · 2017-05-02T06:30:33Z

jerry-port/default/defaultx-extfunc.c

+void
+jerryx_port_extfunc_print_char (char c) /**< the character to print */
+{
+  printf ("%c", c);


I think this function is unnecessary.

Note, that without this function, the 'print' handler does not work. (It cannot have printf hardwired because we have platforms (targets) where printf is unavailable.)

The alternative is to hardwire printf into the print handler, but then we will lose reusability, as many targets will have to copy-paste the code, which we wanted to avoid AFAIK.

Having said that, I'm not sure that this is the best approach. And I'm definitely open to suggestions.

What about jerry_port_console?

We are just removing that in #1749 .

Moreover, the needs of the external print handler are a bit different from the print implementation that we had as a builtin. (The builtin expected a printf-like vararg interface from jerry_port_console, but the current external print handler needs to write a single char only -- which IMHO is good, as vararg turned out to be a pain sometimes and for some targets.)

Moreover 2, jerry_port_console suggests that all ports implement it, but that's not the case. If an app does not use jerry-ext, it will not need to implement jerry_port_console.

zherczeg · 2017-05-02T06:31:41Z

jerry-ext/include/jerryscript-ext/extfunc.h

+ * Common external function handlers
+ */
+
+jerry_value_t jerryx_extfunc_assert (const jerry_value_t func_obj_val, const jerry_value_t this_p,


I don't really like this prefix+prefix notation. I think we have enough names, and there will be no clashes, so this can simply be jerryx_assert

My argument for prefixing: jerry-ext is like multiple libraries merged into one. As if we had one library for argument validation, one for common external function handlers, etc. They all should have their own "namespaces". They already have separate headers (following the pattern "jerryscript-ext/NAMESPACE.h"). IMHO, they should also have their own identifier namespaces: "jerryx_NAMESPACE_whatever".

zherczeg · 2017-05-02T06:34:35Z

jerry-ext/include/jerryscript-ext/extfunc.h

+ */
+
+#ifndef JERRYX_EXTFUNC_H
+#define JERRYX_EXTFUNC_H


As for me jerry-ext/include/jerryscript-ext/extfunc.h is too long. I would prefer:

jerry-ext/include/jerryscript-extfunc.h or jerry-ext/jerryscript-ext/extfunc.h

This is just source tree organization. It will/should not be reflected in the include directives. Basically, "jerry-ext/include" is on the include path and users will still have to use #include "jerryscript-ext/extfunc.h" (or, as another example, #include "jerryscript-ext/args.h" for the argument validator) only in the sources. And having that "include" path component under "jerry-ext" serves two causes: 1) eases header installation with cmake, and 2) consistency (all our libraries follow that sheme -- except for "jerry-core" but I'm trying to fix that up in #1793).

Ok keep it that way but please to find a better name for extfunc.h which describe its purpose.

What about "jerryscript-ext/external.h" for header and jerryx_external_xxx for identifiers? That's just one character longer than extfunc?

Ok for header name. But jerryx_external_ is jerry-external-external

Nope. jerry-extension-external. Much better :)

akosthekiss · 2017-05-02T15:51:07Z

I've updated the PR. The "extfunc" module name got replaced by "external" all over the code.

The PR still does not contain the "infrastructure" (build system) of jerry-ext. IMHO, It would not be fair to rip it off from #1740.

LaszloLango · 2017-05-02T16:34:11Z

jerry-ext/external/external-register.c

+ *         value marked with error flag - otherwise
+ */
+jerry_value_t
+jerryx_external_register_global_function (const jerry_char_t *name_p, /**< name of the function */


I feel a bit redundancy in this kind of naming. I thought x in jerryx is for external. I think external is unnecessary, but this is only my personal preference. I'm not sure was there a final consensus in the naming or not.

We should really write this down at some point of time so that we remember what is what in our chosen naming scheme. In #1740, when jerry-ext vs jerry-util was disputed (for the name of the library), we have decided for ext, to stand for extension (#1740 (comment), #1740 (comment), #1740 (comment)). And identifiers of the jerry-ext library to have jerryx_ prefix (where x still stands for extension).

Yes it is confusing, it can be extension, external, extra.

Maybe jerry-ext/util/global-func.c ?

@akosthekiss I can deal with the current name, but what about jerryx_builtin_ as @zherczeg suggested (#1787 (review)) I think it would be better.

I'm against "builtin": builtin is something that's built in. These functions are not. They are external. The term "external builtin" is self-contradictory.

But I'm also against polluting the "global namespace" of jerry-ext with each and every function. We should keep the concept of "modules" or "sub-libraries" within jerry-ext. The currently proposed one is to deal with (common) external functions.

To me jerryscript-ext/assert.h sounds like a file which defines a macro similar to JERRY_ASSERT rather than providing a callback for a JS function.

This gonna be a long debate on the prefix... Just one more thing to add to the mix: let's not forget that we are using jerry_create_external_function and jerry_external_handler_t to create external functions. I thought that this was an established phrase for functions we bound into the JS scope.

Still LGTM from my side. You may land with the current prefixes if you want.

To me jerryscript-ext/assert.h sounds like a file which defines a macro similar to JERRY_ASSERT rather than providing a callback for a JS function.

Hmm jerryscript-ext/js-assert.h?

LaszloLango

LGTM

martijnthe · 2017-05-03T07:14:55Z

@zherczeg and @akosthekiss I think these two comments are kinds of conflicting:

From all extensions, this should land first because it is simple, and we can define the extension format here.

The PR still does not contain the "infrastructure" (build system) of jerry-ext. IMHO, It would not be fair to rip it off from #1740.

I think @jiangzidong 's PR (#1740) is very close to being done + it includes the build system stuff for jerry-ext. @zherczeg OK to land that first?

zherczeg · 2017-05-03T12:38:11Z

Ok land that first.

akosthekiss · 2017-05-04T14:09:06Z

Now that #1740 has landed, I've rebased this PR on latest master and made use of the jerry-ext infrastructure. On top of the previous commit, there is now also the adaptation of jerry-main to use the implementation of the common JS functions from jerry-ext.

zherczeg

I ma still not happy wit the name of extension, although it is literally correct. In other places of the project these functions are called built-ins, and external builtins sounds better than external extensions. But the patch is ok, and I can live with this name.

zherczeg · 2017-05-06T01:40:11Z

New idea: what about esfunc-assert.h? So we emphasize that it is an ECMAScript function.

zherczeg · 2017-05-06T01:41:07Z

I just noticed that there is no documentation for the new API.

zherczeg

Documentation is missing.

akosthekiss · 2017-05-06T04:51:45Z

@zherczeg To me, esfunc is no better than extfunc that was downvoted originally. And I still don't see why we would need separate headers for each external function handler (we don't have multiple headers for all the argument validation and transform functions).

akosthekiss · 2017-05-06T07:22:58Z

I've added documentation for the extension.

zherczeg · 2017-05-08T13:11:19Z

jerry-ext/external/external-print.c

+#include "jerryscript-ext/external.h"
+
+
+/**


Only one newline.

zherczeg · 2017-05-08T13:12:21Z

jerry-ext/external/external-gc.c

+ * @return undefined.
+ */
+jerry_value_t
+jerryx_external_gc (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */


The __attribute__((unused)) is gcc specific, we should use JERRY_UNUSED()

We don't have access to that here, but I'll rewrite it to (void) (func_obj_val);

zherczeg · 2017-05-08T13:18:03Z

The difference is that external is a generic term, which does not specifiy the nature of these functions at all. This is the same problem with extfunc. The name should emphiasize that these are JS callback functions. In jerry core these are called external callbacks which tries to show their purpose although not the best name I admit (it is a legacy name though).

zherczeg · 2017-05-08T13:23:19Z

What about jerryx_print_handler? handler is a popular term for callback functions

akosthekiss · 2017-05-08T13:35:06Z

For the sake of accuracy, these functions are called "external function handler"s in jerry-core, and their type is jerry_external_handler_t (I haven't seen "callback" in their terminology, neither in API nor internally). The extensions we provide should be consistent with the core in their naming, IMO. That's why "external" was always in the names I proposed, because that's the public API terminology associated with these "things".

As for "jerryx_print_handler": I'll strongly resist against anything unprefixed. Right now, we have arg in jerry-ext, then we have this PR, and contexts are also coming, and who knows what's next. I'll keep insisting to have each "module" within its own "identifier namespace" (+ directory, + header).

Would jerryx_handler_print, jerryx_handler_gc, jerryx_handler_assert, jerryx_handler_register_global, jerryscript-ext/handler.h be better / more favoured / acceptable?

zherczeg · 2017-05-08T13:44:18Z

Probably handler implies callback, since handlers are subset of callbacks. jerryx_handler_print is ok for me. What about plural here: jerryscript-ext/handlers.h ?

akosthekiss · 2017-05-08T13:57:13Z

I'll rename everything to jerryx_handler_ then, if noone objects. Not sure about the plural in the header name. Argument handling extension uses singular, context extension likewise.

akosthekiss · 2017-05-08T14:32:37Z

I've applied the name change, it's in a commit on top of the rest (will squash if everyone is happy with it)

zherczeg

LGTM

Added `handler` module to `jerry-ext` to contain implementation of commonly used external function handlers: `assert`, `gc`, and `print`. Also adapted jerry-main to use jerry-ext/handler JerryScript-DCO-1.0-Signed-off-by: Akos Kiss [email protected]

akosthekiss mentioned this pull request Apr 26, 2017

Remove the built-in print function #1749

Merged

akosthekiss force-pushed the jerryx-extfunc branch from bd3bd97 to 8f51423 Compare April 28, 2017 09:19

zherczeg reviewed May 2, 2017

View reviewed changes

zherczeg requested changes May 2, 2017

View reviewed changes

akosthekiss force-pushed the jerryx-extfunc branch 2 times, most recently from 267f346 to 77d14a4 Compare May 2, 2017 15:44

LaszloLango reviewed May 2, 2017

View reviewed changes

LaszloLango approved these changes May 2, 2017

View reviewed changes

akosthekiss force-pushed the jerryx-extfunc branch from 77d14a4 to 669d454 Compare May 3, 2017 09:54

akosthekiss force-pushed the jerryx-extfunc branch from 669d454 to 7be9b79 Compare May 4, 2017 14:05

akosthekiss force-pushed the jerryx-extfunc branch from 7be9b79 to 2fc41a3 Compare May 4, 2017 14:28

akosthekiss changed the title ~~WIP: common but non-standard JS function implementations in jerry-ext~~ Common but non-standard JS function implementations in jerry-ext May 4, 2017

zherczeg approved these changes May 5, 2017

View reviewed changes

zherczeg requested changes May 6, 2017

View reviewed changes

zherczeg requested changes May 8, 2017

View reviewed changes

akosthekiss force-pushed the jerryx-extfunc branch from 9ecfb52 to e2ba767 Compare May 8, 2017 14:30

zherczeg approved these changes May 9, 2017

View reviewed changes

akosthekiss force-pushed the jerryx-extfunc branch from e2ba767 to f116008 Compare May 9, 2017 07:32

akosthekiss merged commit a8f2d31 into jerryscript-project:master May 9, 2017

akosthekiss deleted the jerryx-extfunc branch May 9, 2017 08:17

		#include "jerryscript-ext/external.h"


		/**

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

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

Uh oh!

Conversation

akosthekiss commented Apr 26, 2017

Uh oh!

zherczeg commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akosthekiss commented May 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

martijnthe commented May 3, 2017

Uh oh!

zherczeg commented May 3, 2017

Uh oh!

akosthekiss commented May 4, 2017

Uh oh!

zherczeg left a comment

Choose a reason for hiding this comment

Uh oh!

zherczeg commented May 6, 2017

Uh oh!

zherczeg commented May 6, 2017

Uh oh!

zherczeg commented May 8, 2017 •

edited

Loading

zherczeg commented May 8, 2017 •

edited

Loading