A tool in jerry-ext: arg validator for binding #1740

jiangzidong · 2017-04-17T07:04:01Z

Updated on April 23th

the patch is updated by combining review comments:

the folder name is jerry-ext, and all types/symbol inside are named by prefix jerryx_
the header is under jerry-ext/include/jerrycript-ext/xxx
the codes for args transformer are in jerry-ext/args/xxx
the unit test codes are in tests/unit-ext/test-ext-xxx

The doc is not added yet, since we don't get consensus.

It provides some APIs for binding developers, so that
they can validate the type of the js argument and convert/assign them
to the native argument.

Related Issue: #1716

It is the first patch of this util, and some Notes are listed below:

Usage, the example is in test/unit-ext/test-ext-args

/**
 * The handler should have following arguments:
 *   this: Ignore.
 *   arg1: Bool.
 *   arg2: Number. It must be strict primitive number.
 *   arg3: String.
 *   arg4: function. It is an optional argument.
 *
 */
static jerry_value_t
test_validator_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */
                        const jerry_value_t this_val, /**< this value */
                        const jerry_value_t args_p[], /**< arguments list */
                        const jerry_length_t args_cnt) /**< arguments length */
{

  bool arg1;
  double arg2 = 0.0;
  char arg3[5];
  jerry_value_t arg4 = jerry_create_undefined ();

  jerryx_arg_t mapping[] =
  {
    /* ignore this */
    jerryx_arg_ignore (),
    /* 1st argument should be boolean */
    jerryx_arg_boolean (&arg1, JERRY_ARG_CONVERSION_YES, JERRY_ARG_REQUIRED),
    /* 2nd argument should be strict number */
    jerryx_arg_number (&arg2, JERRY_ARG_CONVERSION_NO, JERRY_ARG_REQUIRED),
    /* 3th argument should be string */
    jerryx_arg_string (arg3, 5, JERRY_ARG_CONVERSION_YES, JERRY_ARG_REQUIRED),
    /* 4th argument should be function, and it is optional */
    jerryx_arg_function (&arg4, JERRY_ARG_OPTIONAL)
  };

  jerry_value_t is_ok = jerryx_validate_and_assign_all (this_val,
                                                        args_p,
                                                        args_cnt,
                                                        mapping,
                                                        5);

We use functions like jerry_arg_string, jerry_arg_number to create the jerry_arg_t item for each type.

Dont differentiate specific type of number, only double, which is the return value of jerry_get_number_value. Users should handle the conversion between double to any other number types (e.g. int32, uint32, ...) for themselves.
JERRY_ARG_CONVERSION_YES means the js argument can be automatically invoked by toNumber, toString or toBoolean, i.e. both "100" and Number(100) will treated as number 100.
JERRY_ARG_CONVERSION_NO only accept the primitive type. i.e. neither "100" nor Number(100) will be treated as number.
JERRY_ARG_REQUIRED/JERRY_ARG_OPTIOANL indicated whether the argument is mandatory or or optional. If it is optional, then when the js argument is undefined, it will stop the validator/assign but without returning any error.
Currently, string with malloc is not supported, it depends on the implementation of jerry_port_malloc/free
Currently, the error message is quite simple. The detailed error message depends on the sprintf in jerry-libc

martijnthe

Quick first pass at reviewing.
Looking promising :)

martijnthe · 2017-04-18T15:22:18Z

tests/unit/test-util-arg-validation.c

@@ -0,0 +1,156 @@
+/* Copyright JS Foundation and other contributors, http://js.foundation


Move into a new subdirectory ?
tests/unit/jerry-util/arg-validator/**

martijnthe · 2017-04-18T15:25:36Z

jerry-util/CMakeLists.txt

+
+# Source directories
+file(GLOB SOURCE_UTIL arg-validator/*.c)
+


To keep it more modular: I would add another jerry-util/arg-validator/CMakeLists.txt and just recurse into it from here. I'd also create a new target for each util.

martijnthe · 2017-04-18T15:27:00Z

jerry-util/arg-validator/arg-init.inline.h

+ * @return a jerry_arg_t instance.
+ */
+static inline jerry_arg_t
+jerry_arg_custome (void *dest, /**< points to the native argument */


typo: custom

martijnthe · 2017-04-18T15:31:35Z

jerry-util/arg-validator/arg-validator.c

+
+  if (c_arg_cnt == 0)
+  {
+    return ret;


Does it make sense to return jerry_create_undefined() here and not initialize ret in line 80?

martijnthe · 2017-04-18T15:37:38Z

jerry-util/arg-validator/arg-validator.c

+  {
+    iterator.c_arg_p = &c_arg_p[c_arg_index];
+
+    ret = c_arg_p[c_arg_index].func (&iterator);


ret = iterator.c_arg_p->func (&iterator) ?

martijnthe · 2017-04-18T15:43:25Z

jerry-util/arg-validator/arg-validator.h

+/* The argument's type must be the primitive, e.g. number, string, boolean. */
+#define JERRY_ARG_STRICT true
+/* The validator inside will invoke toNumber, toBoolean or toString */
+#define JERRY_ARG_RELAX false


JERRY_ARG_COERCE or JERRY_ARG_LOOSE?

FWIW, MDN talks about "loose equality" and "strict equality": https://developer.mozilla.org/en-US/docs/Web/JavaScript/Equality_comparisons_and_sameness

Not sure if it these are the established terms.

martijnthe · 2017-04-18T15:46:55Z

jerry-util/arg-validator/arg-validator.h

+
+/* Inline functions for initializing jerry_arg_t */
+/* The argument's type must be the primitive, e.g. number, string, boolean. */
+#define JERRY_ARG_STRICT true


strict may sound like it has something to do with use strict; ? Not sure...

martijnthe · 2017-04-18T15:48:40Z

jerry-util/arg-validator/arg-validator.h

+/* The argument is optional */
+#define JERRY_ARG_OPTIOANL true
+/* The argument is required */
+#define JERRY_ARG_REQUIRED false


I'd rather have typedef'd enums for these instead of macros that are bools under the hood.

They work better with code completion.
It also prevents one from using the true and false directly, which makes the code less readable.

martijnthe · 2017-04-18T15:51:02Z

jerry-util/arg-validator/arg-validator.h

+#define JERRY_ARG_REQUIRED false
+static inline jerry_arg_t jerry_arg_number (double *dest, bool is_strict, bool is_optional);
+static inline jerry_arg_t jerry_arg_boolean (bool *dest, bool is_strict, bool is_optional);
+static inline jerry_arg_t jerry_arg_string (char *dest, uint32_t size, bool is_strict, bool is_optional);


Are you planning to create another one for the malloc'd one?

Yes, but firstly, we'd better have jerry_port_malloc in targets, and the malloc in jerry-libc, right?

OK, let's get the base framework merged first and then we can add other transforms like the malloc'd-string one.

martijnthe · 2017-04-18T15:52:35Z

jerry-util/arg-validator/arg-validator.h

+#define JERRY_ARG_OPTIOANL true
+/* The argument is required */
+#define JERRY_ARG_REQUIRED false
+static inline jerry_arg_t jerry_arg_number (double *dest, bool is_strict, bool is_optional);


I'd like to see specific functions for uint8_t, uint16_t, etc. that also perform range checking on the input value.
We can add them later too of course. I think it's nice to have a basic jerry_arg_number though.

martijnthe · 2017-04-18T15:56:01Z

@janjongboom @grgustaf @mjessome any feedback?

martijnthe · 2017-04-18T15:56:36Z

Also @gabrielschulhof any feedback?

zherczeg · 2017-04-19T13:03:55Z

There was an interesting idea here: since we consider 'util' as a key part of jerryscript, extension ('ext' in short) could be a better name. It emphasizes the relation better. What do you think?

martijnthe · 2017-04-19T13:13:25Z

So jerry-ext vs jerry-util?
It a subtle difference, but like it.

Talking about naming.
I'm not too happy with arg-validator.
It doesn't really cover the "transformation" part (from jerry_value_t to C types).
At the core this thing is really just doing array transforms, but I don't think it makes sense to call it after this, because I suspect it won't be used much outside of the context of argument validation & transformation.

How about naming it after existing argument validation / transformation utilities? For example optget, (C), getopts (bash) or argparse (python)? Or is that too confusing? I guess those have connotations of command line utilities, which is not appropriate I think.

Or how about calling it just args?

...?

zherczeg · 2017-04-19T13:17:33Z

jerry-ext or jerry-extension if people likes it.

martijnthe · 2017-04-19T19:19:48Z

@HBehrens any feedback?

jiangzidong · 2017-04-20T01:14:56Z

jerry-ext vs jerry-util:
I can accept both, but I prefer util.
util is more like a toolset where people can get what they needed. ext is like a upgraded/enhanced jerry-core, like support es6, es7 profile or sth.
(Just my thought, and I am not a native speaker.)

arg-validator:
Yes, I agree that the name didn't reflect the conversion part.
What about arg-convert or arg-parse (make it different from python's argparse)? I think we can hide the validate, because it is naturally to validate during conversion.

akosthekiss · 2017-04-20T21:25:18Z

My two cents (comments and questions that may lead to more and more questions):

Naming

Do we want to clutter the jerry_ namespace with types/functions from jerry-util|ext? I'd vote against that. IMHO, let's keep jerry_ for jerry-core.
It would be well aligned with naming conventions from other projects to have jerryx_ prefix for these extension utilities.
If we go this way, then there is no other option than to choose jerry-ext as the toolset/subdir name. (Even though I'm a bit schizophrenic here, as jerry-util sounds much better to me. Unfortunately, jerryu_ does not look good at all.)

Lib structure

Do we want to have multiple libraries for jerry-ext|util? (I see a hierarchy of CMakeLists files in the current version.) I'd vote against that. IMHO, let's have a single archive and let the linker pick the referenced objects and throw away what's not needed. It would let the user keep just one lib file in mind that needs to be added to the dependencies.
Of course, independent parts of the lib might/should be separated into different subdirs to ease maintenance.
How many public headers should we use and where to put them? My first guess would be to suggest multiple headers (per independent parts, mentioned above) under jerryscript-ext/*.h. (Some justification: jerry-api.h has already been obsoleted by jerryscript.h, and I'll/'d suggest - in a different issue/PR - to obsolete jerry-port.h by jerryscript-port.h, too. So, hopefully, jerryscript[-*][/*].h will be our naming scheme in the future.)

Tests

How about not deepening the subdir tree further but adding tests under tests/unit-ext|util?
(Now, I start roaming wild.) This might trigger a bigger refactoring under tests dir to make things consistent, e.g., have tests/unit-core, tests/unit-libm, too.

(Seems that some of the ideas above contradict with previous comments from @martijnthe . Still, I hope that at least some of them have some value. )

jiangzidong · 2017-04-21T01:27:23Z

@akosthekiss
I am ok with jerry-ext and jerryx prefix. Any feedback from others about the naming?

My first guess would be to suggest multiple headers (per independent parts, mentioned above) under jerryscript-ext/*.h

Just to verify, do you suggest a folder layers like jerry-ext/jerryscript-ext/xxx.h (jerryscript-ext is not under jerry-core, right?)

How about not deepening the subdir tree further but adding tests under tests/unit-ext|util

+1, some extended feature of jerry-ext may need function of standard libc (e.g. malloc, sprintf), then we can build the tests/uint-ext with --jerry-libc=off before we support those functions in jerry-libc

glistening · 2017-04-21T01:43:55Z

+1 for jerryx_. It's good idea to keep jerry_ prefix for jerry_core.

zherczeg · 2017-04-21T03:04:15Z

For paths:

I would separate the public and private header of extensions. An example:

All public headers would go:

/jerry-ext/include/std-module-manager.h

All the rest would go:
/jerry-ext/std-module-manager/std-module-manager-a.c
/jerry-ext/std-module-manager/std-module-manager-a.h
/jerry-ext/std-module-manager/std-module-manager-b.c
/jerry-ext/std-module-manager/std-module-manager-b.h
....

This way people would need to pass -I/jerry-ext/include/ directory for their project, and link it with a single archive.

Extensions should not depend on each other by default, and if there is a dependency, it should be clearly stated in the extension readme. This way the linker could easily discard the unneded functions.

As for unit tests:
tests/unit-ext/std-module-manager/tests

I know this require a bit more work initially, but when we have dozens of extensions it will worth it.

LaszloLango · 2017-04-21T06:32:00Z

jerry-ext and jerryx_ are good to me. The suggested paths by @zherczeg sounds good too.

akosthekiss · 2017-04-21T09:20:40Z

Re: "header paths" @jiangzidong @zherczeg
I've been thinking a bit further ahead and not clearly expressed that. I've been thinking about a use case typical for desktops, where libraries are already built and available in binary+headers form only, installed to standard locations. (Actually, even embedded build setups might work that way, but none of our in-tree targets do, AFAIK.) Then, IMHO, it is not good practice to clutter the global include namespace with headers like std-module-manager.h. It's much friendlier to add headers under a subdirectory (strictly or loosely) named after the project. So, users of the library can write:

#include "jerryscript.h"
#include "jerryscript-port.h"
#include "jerryscript-ext/std-module-manager.h" // might cooperate better with other projects
//#include "std-module-manager.h" // more prone to name collisions

That could mean path structure for public headers either
/jerry-ext/include/jerryscript-ext/std-module-manager.h
or
/jerry-ext/jerryscript-ext/std-module-manager.h
(The first one being better, perhaps.)

martijnthe · 2017-04-21T09:48:49Z

Good feedback, thanks.

Do we want to have multiple libraries for jerry-ext|util? (I see a hierarchy of CMakeLists files in the current version.) I'd vote against that. IMHO, let's have a single archive and let the linker pick the referenced objects and throw away what's not needed. It would let the user keep just one lib file in mind that needs to be added to the dependencies.

Yeah, that has merit as well, so SGTM.

arg-convert

That gets a bit long...
How about just args / jerryx_args_... ?

For this particular util, I think it's probably a good idea to group transformer functions + helpers in separate header files, because there are already quite many of them and more can/will be added later.
So, how about:

// main header, contains `jerryx_args_validate_and_assign_all`, main types and some common converters.
#include "jerryscript-ext/args.h" 

// contains converters for uint8_t, uint16_t, etc:
#include "jerryscript-ext/args/stdint.h"  //<< is naming the file stdint.h asking for trouble?

// etc.
#include "jerryscript-ext/args/futurexyz.h"

akosthekiss · 2017-04-21T09:57:38Z

@martijnthe alternative suggestion:

#include "jerryscript-ext/args-stdint.h" // note the dash instead of the slash

We should be able to manage our own name conflicts, so no need for include paths deeper than 2, IMHO.

martijnthe · 2017-04-21T11:03:02Z

#include "jerryscript-ext/args-stdint.h" // note the dash instead of the slash

Works for me.

akosthekiss · 2017-04-21T12:37:48Z

The two above-mentioned refactoring ideas (organizing unit tests into separate subdirs per component, and naming all existing public headers as jerryscript[-*].h) got their own PRs: #1761 and #1762 .

martijnthe · 2017-04-21T17:39:31Z

I think we also need docs with utils.
Any preferences where those should live?

Some options:

/jerry-ext/std-module-manager/docs/std-module-manager.md
/jerry-ext/docs/std-module-manager.md
/docs/jerry-ext/std-module-manager.md

akosthekiss · 2017-04-21T17:43:55Z

I'm not sure that /docs is ready for that... If I had to vote, I'd vote for the second option (/jerry-ext/docs/...) but that's not a strong opinion

LaszloLango · 2017-04-21T17:44:36Z

Every documentation should be created into the docs folder in the root. Creating a subfolder for jerry-ext is acceptable for me.

martijnthe · 2017-04-21T17:47:36Z

I'm not sure that /docs is ready for that..

Can you elaborate what you mean with this?

akosthekiss · 2017-04-21T18:19:14Z

@martijnthe Sure. Sorry for my brevity. As it currently stands /docs is quite a strictly organized directory, with chapter-like, numbered-and-all-caps files, that are also exported to the web page. The example shown in the third option looked like a much less strictly controlled documentation. To me, at least.

But I could get both the current setup and the here-presented intent wrong.

(Note: and I'm not sure why, having jerry-ext under docs looks weird to me. But that's really subjective.)

Let me add a fourth option:

/docs/ext/std-module-manager.md

martijnthe · 2017-04-21T18:26:53Z

As it currently stands /docs is quite a strictly organized directory, with chapter-like, numbered-and-all-caps files, that are also exported to the web page.

Gotcha. Thanks for explaining.
How about adding to the existing docs?
I kind of like the idea of the docs for the utils also being available through the web page.

akosthekiss

Some extra notes (learned during experiments)

akosthekiss · 2017-05-02T22:57:08Z

jerry-ext/args/arg-iterator-helper.c

+  }
+
+  jerry_value_t ret = *iterator_p->js_arg_p;
+  iterator_p->js_arg_index++;


IMO, incrementing the index should happen even if it grows higher than cnt.

akosthekiss · 2017-05-02T22:59:37Z

jerry-ext/args/arg-transform-functions.c

+#define JERRYX_ARG_TRANSFORM_DEF(type) jerry_value_t \
+ jerryx_arg_transform_ ## type (jerryx_arg_iterator_t *iterator_p) \
+{ \
+ return jerryx_arg_transform_ ## type ## _impl (iterator_p); \


I see no need for these trampolines. The actual implementations don't really need the _impl suffix, they simply don't have to be static.

akosthekiss · 2017-05-02T23:06:53Z

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

+/**
+ * Signature of the transform funtion.
+ */
+typedef jerry_value_t (*jerryx_arg_transform_func_t) (jerryx_arg_iterator_t *iterator_p);


this is very personal taste (and signature will change anyway), but the name of the function parameter should not be simply iterator_p, which just repeats the name of the type of the parameter, but should also tell something about the data (e.g., in the previously mentioned commits, js_args was used to highlight that it's an iterator over the JS arguments -- not necessarily the best name, could be js_arg_it, or ...)

I like your idea about the macro helper. what about the js_arg_iter_p

#define JERRYX_ARG_TRANSFORM_FUNC(NAME) \ jerry_value_t NAME (jerryx_arg_js_iterator_t *js_arg_iter_p, \ const jerryx_arg_t *c_arg_p)

akosthekiss · 2017-05-02T23:07:17Z

jerry-ext/include/jerryscript-ext/args.impl.h

+jerry_value_t jerryx_arg_transform_function (jerryx_arg_iterator_t *iterator_p);
+jerry_value_t jerryx_arg_transform_function_optional (jerryx_arg_iterator_t *iterator_p);
+jerry_value_t jerryx_arg_transform_constructor (jerryx_arg_iterator_t *iterator_p);
+jerry_value_t jerryx_arg_transform_constructor_optional (jerryx_arg_iterator_t *iterator_p);


these two functions have no implementation

jiangzidong · 2017-05-03T02:24:25Z

@akosthekiss I cherry-picked your commits and add patch based on it, so you can see the differences clearly.

akosthekiss

minor comments (starts looking good to me)

an open question: shall we or shall we not split the source files into smaller ones? (pro: one/few-function-per-object helps the linker. con: maintainability.)

akosthekiss · 2017-05-03T06:19:38Z

jerry-ext/args/arg-transform-functions.c

+ * Get the current JS argument from the iterator.
+ *
+ * Note:
+ *     Unlike jerryx_arg_iterator_pop, it will not change index and


the reference should be updated (jerryx_arg_js_iterator_pop)

akosthekiss · 2017-05-03T06:21:52Z

jerry-ext/args/arg-transform-functions.c

+
+/**
+ * The common function to deal with optional arguments.
+ */


It's my bad to remove the @return comments from most of the functions (because I've changed the return types too often during experiments and couldn't keep them updated). We might consider adding them back as it seems that we have consensus on the signature.

Oh, I missed this one

akosthekiss · 2017-05-03T06:22:37Z

jerry-ext/args/arg-transform-functions.c

+} /* JERRYX_ARG_TRANSFORM_FUNC */
+
+/**
+ * Define tranformer for optional argument.


typo: transformer

akosthekiss · 2017-05-03T06:26:05Z

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

+                                                      const jerryx_arg_t *c_arg_p); /**< native arg */
+
+/**
+ * Helper macro to declare a transform function with proper signature.


We might need to add some note on what parameter names it declares. (As this macro is in a public header, other may also use it, so it becomes part of the public API.)

akosthekiss · 2017-05-03T06:33:05Z

jerry-ext/args/arg-transform-functions.c

+ * @return the current JS argument.
+ */
+jerry_value_t
+jerryx_arg_js_iterator_pop (jerryx_arg_js_iterator_t *js_arg_iter_p) /**< the JS arg iterator */


note: I've moved the push and pop functions into this source file only because I've previously deleted the arg-iterator-helper.c file during my experiments and was too lazy to re-add it. right now, no bias towards any approach (keep it here or split it out).

I will choose to put those jerryx_arg_js_iterator_xxx functions to a arg_js_iterator to keep the maintainability.
And in the future, if we add lots of builtin transform functions, we may also seperate them into different source file according to some similarity, for example, arg-transform-number.c. arg-transform-string.c ...

OK with me (just don't use underscores in the reintroduced file name but hyphens)

akosthekiss · 2017-05-03T06:34:22Z

jerry-ext/args/arg-transform-functions.c

+ * The common function to deal with optional arguments.
+ */
+static jerry_value_t
+jerryx_arg_transform_optional (jerryx_arg_js_iterator_t *js_arg_iter_p, /**< available JS args */


should we not expose this function as public API, too? could someone reuse this during custom transform implementation?

akosthekiss · 2017-05-03T07:23:42Z

I'd suggest squashing the current 4 commits and rebasing to latest master on the next update so that we can get a better look at the PR status.

martijnthe · 2017-05-03T10:37:26Z

@akosthekiss @LaszloLango : re. documentation: I'll be writing some docs for this feature.

I can take care of updating the hierarchy of the web page to accommodate for jerry-ext.
Thoughts on how to structure this?

Maybe make "API Reference" a dropdown (like "Documents") and have a "Core" menu option for the jerryscript.h docs and below that menu options for each util?

Examples for the utils could go on the "API Examples" page, but it's getting a bit long. Maybe also make "API Examples" a menu?

martijnthe · 2017-05-03T11:09:53Z

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

+  const jerry_value_t *js_arg_p; /**< the JS arguments */
+  const jerry_length_t js_arg_cnt; /**< the total num of JS arguments */
+  jerry_length_t js_arg_idx; /**< current index of JS argument */
+} jerryx_arg_js_iterator_t;


Still need to forward declare here in the .h and move the type definition into the .c file.

maybe it is better to stay in the public header, because the custom_transformer_function will use the iterator. If we forward declare it, people can only use the iterator-helpers to handle it.

We can force users to access the iterator via helpers only, that may not be a big problem, the already implemented transform functions did not need direct access to the fields either. But then we also need a helper function to create an interator instance, because jerryx_arg_transform_args and jerryx_arg_transform_args_with_this do need construction.

zherczeg · 2017-05-03T12:40:51Z

jerry-ext/args/arg-transform-functions.c

+#include "jerryscript.h"
+
+
+/**


Two newlines.

zherczeg · 2017-05-03T12:42:58Z

jerry-ext/args/arg-transform-functions.c

+/**
+ * Tranform a JS argument to a double. Type coercion is allowed.
+ */
+JERRYX_ARG_TRANSFORM_FUNC (jerryx_arg_transform_number)


I don't see the point of using a macro here. It makes hard to see the arguments.

@akosthekiss what's your opinion about the macro?

To me, personally, it was a big help. We have cca. 30 functions with identical signature. This macro helped me a lot in ensuring that they are really identical. But I can accept a verdict that function declarations should not be hidden by macros, too. It will be tedious to rewrite, but it will have to be done only once. Hopefully.

I will rewrite them

The compiler checks function definitions and warns if you pass a wrong function. Since all conversion functions should be covered by tests, we would get a compiler error if something is wrong. If we change the macro, we need to rewrite all function bodies, so probably the header is the least problem.

zherczeg · 2017-05-03T12:46:23Z

tests/unit-ext/test-common.h

+
+
+
+#define TEST_ASSERT(x) \


Lots of unneded newlines in this file.

zherczeg · 2017-05-03T12:48:10Z

tests/unit-ext/test-ext-args.c

+  return jerry_create_undefined ();
+
+
+} /* test_validator2_handler */


Lots of unnecessary newlines here as well.

zherczeg · 2017-05-03T12:58:41Z

jerry-ext/args/args.c

+ *         jerry error: the validator fails.
+ */
+jerry_value_t
+jerryx_arg_transform_args_with_this (const jerry_value_t this_val, /**< the this_val for the external function */


to me transform sounds like changing something to something else, like changing a sphere to a cube. This function sounds like conversion (same thing, different form). What about jerryx_arg_get ?

Hmm get is too non-descript if you ask me.

I'd be fine with jerryx_arg_convert_this_and_args and jerryx_arg_convert_args.
But I think it's a minimal difference.

Why do we need to emphasize the this? Do we have a variant without this?

Yes: jerryx_arg_transform_args

Do we have a variant without this?

Yes. Having the "primitive" w/o the this special casing is pretty useful for various reasons.

@jiangzidong BTW, looking at the current name, it's probably best to put this before args in the name, because this is processed as the first value.

Question: it seems the first argument is ignored in this code, which is probably the this, doesn't it? What is the point of having a variant without this when it can be skipped easily?

What is the point of having a variant without this when it can be skipped easily?

It's useful, for example:

Generally, if you don't care about this. No need to pass it, no need to create the "ignore", no need to run the this handling code, etc...

Another concrete use case: inside a (custom) transform function that converts multiple args (but no this) in one step by calling jerryx_arg_transform_args inside the transform.

An extra question to the naming dispute (somewhat unrelated to the above): why do we have jerry-ext/args and jerryscript-ext/args.h but have everything prefixed with jerryx_arg_ ? (Sorry for the late hit.)

I should be arg, I will change them

zherczeg · 2017-05-03T13:02:50Z

tests/unit-ext/test-ext-args.c

+    /* 3th argument should be string */
+    jerryx_arg_string (arg3, 5, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED),
+    /* 4th argument should be function, and it is optional */
+    jerryx_arg_function (&arg4, JERRYX_ARG_OPTIONAL)


This seems very clever but also very verbose (requires a lot of instructions to generate this structure). Have you considered a scanf like interface (I haven't read all comments)?

Do you mean create some specifiers to indicate different types? But in that way, users need to learn the meaning of the specifier, and code is hard to read.

@zherczeg we discussed that idea, but the current solution allows the linker to strip out any logic that's not used. A scanf like function would have a big switch inside which would pull in all transforms. Plus it's not as easily extensible as this solution.

Re. "lots of instructions": @jiangzidong analyzed this solution and it's not that bad. The break-even point w.r.t. code space at the call-site, compared to manual checking, lies between 1-2 arguments. So if a function needs to check more than 1 argument, this solution is already better from a code size point of view than manually writing the validation.

It's also really simple to add helpers on top to make it less verbose if one really wants to.

Yes, no scanf please. All those jerry_arg_TYPE functions are static inlines which do nothing but construct/fill a struct. All the JERRYX_ARG_COERCE,REQUIRED,OPTIONAL parameters will be constant-propagated and optimized out, so no function call overhead is involved.

I know that. The problem is that the compiler allocates a big structure on the stack (keeps it until the function exits, although it provides nothing useful after the conversion is completed), initialize it with several instructions (not a setmem zero). However I do understand this is quite user friendly, so we can keep it. We can also add other argument parsers later for less generic cases.

allocates a big structure on the stack

I thought it might be possible to make the jerryx_arg_t mapping[] a static const to avoid the stack issue, because the elements are actually all constant data.
Unfortunately the compiler isn't smart enough: error: initializer element is not a compile-time constant.
I guess if we went with macros instead of inline functions to create the jerryx_arg_ts, making it static const would be possible.

@martijnthe the addresses of local variables would still not be compile time constants, would they?

Oh duh.. you're right.

akosthekiss · 2017-05-03T13:59:44Z

jerry-ext/args/arg-js-iterator-helper.c

+ * @return the index
+ */
+jerry_length_t
+jerryx_arg_js_iterator_get_index (jerryx_arg_js_iterator_t *js_arg_iter_p) /**< the JS arg iterator */


suggestion to shrink the utility function name a bit: _get_index -> _index ?

akosthekiss · 2017-05-03T14:05:25Z

jerry-ext/args/args-internal.h

+/**
+ * The iterator structor for JS arguments.
+ */
+struct jerryx_arg_js_iterator_t


Another place for declaring the struct would be arg-js-iterator-helper.c and not even having an internal header. This would ensure that even the other components of this library would be blocked from accessing the internals of the data structure and would be forced to use the accessor methods. I'm not insisting but that would be perhaps the "nicest" or "cleanest" approach.

Sorry I don't understand. the other components of this library can access to the internal iterator as long as they include this header args-internal.h

((Sure. The question is whether we want to allow them that? Whether we want to have a truly opaque iterator type or one that is only opaque to the users of the library? Even in the current for of the PR, arg-transform-functions.c includes args-internal.h but does not use it (all peeks and pops are performed with the help of helpers). Other than arg-js-iterator-helper.c, only args.c uses the knowledge of the internals of the iterator type, when jerryx_arg_transform_args_with_this and jerryx_arg_transform_args construct iterators. If we had one more helper -- say, jerryx_arg_js_iterator_create,construct,init --, then not even args.c had to know how the iterator type looks like.))

Scratch all that. That's not true. Because jerryx_arg_transform_args_with_this and jerryx_arg_transform_args have the iterator in a local variable (not just a pointer to an iterator), they have to know the size of the type, so they have to see the full declaration.

We do need the internal header. (But we don't necessarily have to include it in arg-transform-functions.c as it's unused there, that still holds.)

zherczeg · 2017-05-03T14:19:13Z

After removing the macro and removing those unnecessary newlines (in many files) I am fine with this patch. I will likely not be a user for this which makes judging things a bit difficult.

jiangzidong · 2017-05-03T16:04:02Z

@zherczeg @akosthekiss : I addressed the above comments and @martijnthe added the document

akosthekiss

Spotted some doc issues. (But note, I'm not a master of docs.)
@zherczeg @LaszloLango @bzsolt Is that OK with you as is?

akosthekiss · 2017-05-03T16:06:48Z

docs/09.EXT-REFERENCE-ARG.md

+
+The structure defining a single validation/transformation step.
+
+*Note*: For commonly used validators, `args.h` provides helpers to create the `jerryx_arg_t`s.


args.h was renamed to arg.h

akosthekiss · 2017-05-03T16:12:07Z

docs/09.EXT-REFERENCE-ARG.md

+ a successful transformation into `c_arg_p->dest`. In case the validation did
+ not pass, the transform should not modify `c_arg_p->dest`.
+
+ Additional parameters can be provided to the function through `c_arg_p->extra_info`.


intentional indentation in these 3 lines?

akosthekiss · 2017-05-03T16:13:59Z

docs/09.EXT-REFERENCE-ARG.md

+    jerryx_arg_ignore (),
+
+    jerryx_arg_boolean (&required_bool, JERRYX_ARG_NO_COERCE, JERRYX_ARG_REQUIRED),
+    jerryx_arg_string (&required_str, sizeof(required_str) JERRYX_ARG_NO_COERCE, JERRYX_ARG_REQUIRED),


missing comma

akosthekiss · 2017-05-03T16:17:35Z

docs/09.EXT-REFERENCE-ARG.md

+  // Validate and transform:
+  const jerry_value_t rv =
+      jerryx_arg_transform_this_and_args (this_val, args_p, args_count,
+                                          mapping, ARRAY_LENGTH(mapping));


this statement is out of style (= and line break, no space before ( )

akosthekiss · 2017-05-03T16:31:45Z

docs/09.EXT-REFERENCE-ARG.md

+ - return value - the `jerry_value_t` argument that was popped.
+ - `js_arg_iter_p` - the JS arg iterator from which to pop.
+
+## jerryx_arg_js_iterator_peek


should not jerryx_arg_js_iterator_index be documented, too?

sure, will add it soon

akosthekiss · 2017-05-03T16:36:08Z

jerry-ext/arg/arg-js-iterator-helper.c

+ * @return the index
+ */
+jerry_length_t
+jerryx_arg_js_iterator_get_index (jerryx_arg_js_iterator_t *js_arg_iter_p) /**< the JS arg iterator */


keeping the longer name?

oh, forgot to change that

akosthekiss

LGTM (very well done IMO)

martijnthe · 2017-05-04T07:22:56Z

LGTM (very well done IMO)

🎉

@zherczeg @LaszloLango @bzsolt OK to merge?

LaszloLango · 2017-05-04T07:50:38Z

As I see you've got one LGTM and @zherczeg has an in-progress review, so it depends on him. :)

zherczeg

LGTM. The code is surprisingly complex, so it would still be a good idea to create a simplified, scanf like interface for simple argument parsing.

zherczeg · 2017-05-04T07:48:52Z

docs/09.EXT-REFERENCE-ARG.md

+                                                               mapping,
+                                                               ARRAY_LENGTH (mapping));
+
+  if (jerry_value_has_error_flag (rv)) {


Move { to the next line.

jiangzidong · 2017-05-04T08:06:40Z

@zherczeg What's scanf-like style ? like that?

{
int x;
bool y;
char z[10];
void * c_args[3] = {&x, &y, z}
uint8_t type[3] = {NUM_STRICT, BOOL_STRICT_OPTIONAL, STRING_LOCAL} // they are enums
is_ok = transform_args (type, c_args, js_args, 3);
}

It provides some APIs for binding developers, so that they can validate the type of the js argument and convert/assign them to the native argument. Related Issue: jerryscript-project#1716 JerryScript-DCO-1.0-Signed-off-by: Martijn The [email protected] JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

zherczeg · 2017-05-04T08:38:11Z

Something like:

int x;
bool y;
char z[10];
jerry_value_t func;

transform_args ("Si Ob s10 f", js_args, 3, &x, &y, &z, &func)

Space would separate parameters to be readable. Flags that can be before a type:
O - optional (there would be a default value for each type, e.g. false for boolean and 0 for number)
S - strict: conversion (coerce) is NOT allowed (most of the time you don't need strict arguments)

i (int), b (bool), s (string) (space follows s as a decimal number), o (object), on (object or null).

This would focus on simple cases rater than super generic transformations, but would work 90% of the time. Of course we could use enums as well and combine strings, but this would make this too long:

#define JERRYX_ARG_INT "i"

If there is an error, all data is freed (so you don't need to explicitly free successfully parsed jerry_value_t types).

martijnthe · 2017-05-04T08:52:25Z

Yeah, that's what I understood when you mentioned scanf.
This could be built on top of what we've got now.
As I mentioned before, the parser for the "format string" would drag in all the possible transformer code, which is why I don't like this solution. That said, at the call site, this has most likely an even slimmer code size footprint.

jiangzidong · 2017-05-04T08:52:57Z

Thanks for your explanation, we will think about that in the following patches.

akosthekiss · 2017-05-04T09:35:44Z

@jiangzidong @martijnthe I think you are all set to push the big ~~red~~ green button (I can do it for you, too, but don't want to steal the joy)

jiangzidong · 2017-05-04T10:00:29Z

@akosthekiss Haha, thanks :)

martijnthe · 2017-05-04T10:02:41Z

🎆 🎉

jiangzidong mentioned this pull request Apr 17, 2017

[Proposal] Utilities for argument validation in external functions #1716

Closed

jiangzidong force-pushed the argvalidation branch from 41ec130 to 2c55f8f Compare April 18, 2017 01:57

martijnthe reviewed Apr 18, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from afd278c to 685d6b9 Compare April 19, 2017 06:45

akosthekiss mentioned this pull request Apr 21, 2017

Split unit tests into separate directories #1761

Merged

jiangzidong force-pushed the argvalidation branch from 685d6b9 to 77299c8 Compare April 23, 2017 05:57

jiangzidong changed the title ~~A tool in jerry-util: arg validator for binding~~ A tool in jerry-ext: arg validator for binding Apr 23, 2017

akosthekiss reviewed May 2, 2017

View reviewed changes

akosthekiss reviewed May 3, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from 381f541 to 6bd3e87 Compare May 3, 2017 08:26

martijnthe reviewed May 3, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from 6bd3e87 to 575b03b Compare May 3, 2017 12:39

zherczeg reviewed May 3, 2017

View reviewed changes

akosthekiss reviewed May 3, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from d1bc43a to 45a6de9 Compare May 3, 2017 16:02

akosthekiss reviewed May 3, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from 45a6de9 to fa78ade Compare May 4, 2017 00:58

akosthekiss approved these changes May 4, 2017

View reviewed changes

zherczeg approved these changes May 4, 2017

View reviewed changes

jiangzidong force-pushed the argvalidation branch from fa78ade to 02af6af Compare May 4, 2017 08:01

jiangzidong merged commit 90efdf9 into jerryscript-project:master May 4, 2017

jiangzidong deleted the argvalidation branch May 4, 2017 10:00

		@@ -0,0 +1,156 @@
		/* Copyright JS Foundation and other contributors, http://js.foundation


		# Source directories
		file(GLOB SOURCE_UTIL arg-validator/*.c)

		return jerry_create_undefined ();


		} /* test_validator2_handler */


		The structure defining a single validation/transformation step.

		Note: For commonly used validators, `args.h` provides helpers to create the `jerryx_arg_t`s.

A tool in jerry-ext: arg validator for binding #1740

A tool in jerry-ext: arg validator for binding #1740

Uh oh!

Conversation

jiangzidong commented Apr 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnthe left a comment

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!

martijnthe commented Apr 18, 2017

Uh oh!

martijnthe commented Apr 18, 2017

Uh oh!

zherczeg commented Apr 19, 2017

Uh oh!

martijnthe commented Apr 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zherczeg commented Apr 19, 2017

Uh oh!

martijnthe commented Apr 19, 2017

Uh oh!

jiangzidong commented Apr 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

akosthekiss commented Apr 20, 2017

Uh oh!

jiangzidong commented Apr 21, 2017

Uh oh!

glistening commented Apr 21, 2017

Uh oh!

zherczeg commented Apr 21, 2017

Uh oh!

LaszloLango commented Apr 21, 2017

Uh oh!

akosthekiss commented Apr 21, 2017

Uh oh!

martijnthe commented Apr 21, 2017

Uh oh!

akosthekiss commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnthe commented Apr 21, 2017

Uh oh!

akosthekiss commented Apr 21, 2017

Uh oh!

martijnthe commented Apr 21, 2017

Uh oh!

akosthekiss commented Apr 21, 2017

Uh oh!

LaszloLango commented Apr 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martijnthe commented Apr 21, 2017

Uh oh!

jiangzidong commented Apr 17, 2017 •

edited

Loading

martijnthe commented Apr 19, 2017 •

edited

Loading

jiangzidong commented Apr 20, 2017 •

edited

Loading

akosthekiss commented Apr 21, 2017 •

edited

Loading

LaszloLango commented Apr 21, 2017 •

edited

Loading

jiangzidong May 3, 2017 •

edited

Loading