add transform functions for integer in jerryx/arg #1883

jiangzidong · 2017-06-07T08:36:49Z

JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

martijnthe · 2017-06-07T08:44:59Z

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

+GENERATE_JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (uint16, 0, 65535)
+GENERATE_JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (int16, -32767, 32767)
+GENERATE_JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (uint32, 0, 4294967295)
+GENERATE_JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (int32, -2147483648 , 2147483647)


Use the macros from stdint.h, (U)INT..._MAX/MIN ?

Agree, My concern was that the target may not have <stdint.h>, but now I realized that if so, there is also no such uint8_t things, and people should write their own stdint header.

martijnthe · 2017-06-07T08:49:41Z

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

+  if (!jerry_value_has_error_flag (rv)) \
+  { \
+    tmp = jerryx_arg_clamp_double (tmp, min, max); \
+    *(type ## _t *) c_arg_p->dest = (type ## _t) tmp; \


Should we round?

What do you think? I think they both make sense if we tell the rules to user.

hmm, yeah depends on the use case..
Maybe add a enum new argument to the jerryx_arg_(u)int... helpers, with JERRYX_ARG_ROUND / JERRYX_ARG_FLOOR (could provide ..._CEIL later if someone needs it...)?

I guess some in use cases one may also not want the implicit clamping, but instead throw an error if the input value is out of the possible range.

So, to summarize we've got these "dimensions" when converting numbers:

target storage type (width & signed-ness)

rounding behavior (floor, round or ceil)

out-of-range values: clamping vs throwing Range error

... and then the common dimensions that aren't specific to numbers:
4. optionality
5. strictness

I think it's important to be able to cater for 2 & 3.
But adding 2 & 3 as arguments jerryx_arg_(u)int... will make the signature quite long.
Maybe it makes sense to pass a "config" struct and choose default values such that passing {} as config is a reasonable thing to do?

typedef struct { jerryx_arg_coerce_t coercion; jerryx_arg_optional_t optionality; jerryx_arg_rounding_t rounding; jerryx_arg_clipping_t clipping; } jerryx_arg_number_options_t; //------ uint8_t d = 0; jerryx_arg_t mapping[] = { // {} == "defaults": optional arg + coerce types + round + clip. jerryx_arg_uint8(&d, {}) }; //------- uint8_t d = 0; jerryx_arg_t mapping[] = { // {} == "defaults", all zero jerryx_arg_uint8(&d, (jerryx_arg_number_options_t) { .clipping = JERRYX_ARG_NO_CLIPPING, .coercion = JERRYX_ARG_NO_COERCE }); };

We have 3 kinds of rounding policies and 2 kinds of out-of-range policies. What about define 3*2 = 6 enums. like JERRYX_ARG_INT_ROUND_ERROR (means will round and will throw error if out-of-range ), and passing the enum to the c_arg_p->extra_info.

Then in the validator funtion (e.g. jerryx_arg_transform_uint8_strict), we can choose different policy depends on the c_arg_p->extra_info.
In this way, we don't need to provide 24 functions for one int type, just 4 is enough.

I think this way we can have just one internal transform function for all the int types.

Do you mean put "target storage type" , "optionality" and "strictness" into the jerryx_arg_number_options_t?
The reason why using multiple transform functions for different type is that some unused functions will not linked into the final exe file, so that the binary size can be saved.

Ok, yeah that makes sense.

Still, from a usage point of view, I think it's nice to have a struct where {} results in using reasonable "default" options.

I cannot just pass {} like jerryx_arg_uint8(&d, {})
In C, the braced list of initializers cannot be empty.

It should be jerryx_arg_uint8(&d, (jerryx_arg_int_option_t) {0})

Sorry, I forgot the cast. (jerryx_arg_int_option_t) {} should work too I think.

No, C doesn't support the empty initializer. C++ does.

http://en.cppreference.com/w/c/language/struct_initialization

In C, the braced list of initializers cannot be empty (note that C++ allows empty lists, and also note that a struct in C cannot be empty): struct {int n;} s = {0}; // OK struct {int n;} s = {}; // Error: initializer-list cannot be empty struct {} s = {}; // Error: struct cannot be empty, initializer-list cannot be empty

LaszloLango · 2017-06-07T08:55:14Z

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

-                                    const jerryx_arg_t *c_arg_p) /**< the native arg */
+static jerry_value_t
+jerryx_arg_transform_number_strict_common (jerryx_arg_js_iterator_t *js_arg_iter_p, /**< available JS args */
+                                           double *number_p) /**< OUT: the number in JS arg */


/**< [out] the number in JS arg */

LaszloLango · 2017-06-07T08:59:43Z

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

-                             const jerryx_arg_t *c_arg_p) /**< the native arg */
+static jerry_value_t
+jerryx_arg_transform_number_common (jerryx_arg_js_iterator_t *js_arg_iter_p, /**< available JS args */
+                                    double *number_p) /**< OUT: the number in JS arg */


/**< [out] the number in JS arg */

LaszloLango · 2017-06-07T09:00:18Z

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

+
+/**
+ * clamp a double by given min and max value
+ * @return clamped double value


missing newline before @return

LaszloLango · 2017-06-07T09:00:39Z

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

+} /* jerryx_arg_transform_number_common */
+
+/**
+ * clamp a double by given min and max value


Clamp a double by given min and max value.

jiangzidong · 2017-06-08T04:40:14Z

updated @LaszloLango @martijnthe

jiangzidong · 2017-06-08T12:09:41Z

CI reports

error: type of bit-field ‘round’ is a GCC extension [-Werror=pedantic]
   jerryx_arg_round_t round:2; /**< rounding policy */
   ^

But **in my local machine, it can pass all the tests ** (gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4))

In order to solve this "pedantic" error, I use packed enum instead of bit-field.

martijnthe · 2017-06-08T13:12:16Z

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

+{
+  jerryx_arg_round_t round; /**< rounding policy */
+  jerryx_arg_clamp_t clamp; /**< clamping policy */
+} jerryx_arg_int_option_t;


Maybe it makes more sense to add the packed to the struct type.
AFAIK packed isn't applied to "parent" types.

jiangzidong · 2017-06-13T00:41:17Z

@LaszloLango Furthur comments?

LaszloLango · 2017-06-13T07:19:54Z

docs/09.EXT-REFERENCE-ARG.md

@@ -98,6 +98,68 @@ Enum that indicates whether an argument is optional or required.
 - [jerryx_arg_function](#jerryx_arg_function)
 - [jerryx_arg_native_pointer](#jerryx_arg_native_pointer)

+## jerryx_arg_round_t
+
+Enum that indicates the rounding policy which will be choose to transform a integer.


an integer

LaszloLango · 2017-06-13T07:22:06Z

docs/09.EXT-REFERENCE-ARG.md

+**Summary**
+
+The structure indicates the options used to transform integer argument.
+It will be passed into jerryx_arg_t's  extra_info filed.


LaszloLango · 2017-06-13T07:28:51Z

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

-                                    const jerryx_arg_t *c_arg_p) /**< the native arg */
+static jerry_value_t
+jerryx_arg_transform_number_strict_common (jerryx_arg_js_iterator_t *js_arg_iter_p, /**< available JS args */
+                                           double *number_p) /**< [out]: the number in JS arg */


colon is not needed after [out]

LaszloLango · 2017-06-13T07:31:37Z

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

-                             const jerryx_arg_t *c_arg_p) /**< the native arg */
+static jerry_value_t
+jerryx_arg_transform_number_common (jerryx_arg_js_iterator_t *js_arg_iter_p, /**< available JS args */
+                                    double *number_p) /**< [out]: the number in JS arg */


LaszloLango · 2017-06-13T07:33:32Z

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

 } /* jerryx_arg_transform_number */

 /**
+ * clamp a double by given min and max value.


LaszloLango · 2017-06-13T07:34:08Z

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

+/**
+ * round a double
+ *
+ * @return


Please fix the description

LaszloLango · 2017-06-13T07:34:20Z

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

+} /* jerryx_arg_helper_round */
+
+static jerry_value_t
+jerryx_arg_helper_process_double (double *d,


missing comments

jiangzidong · 2017-06-13T11:17:30Z

@LaszloLango updated

LaszloLango · 2017-06-13T11:36:14Z

docs/09.EXT-REFERENCE-ARG.md

+
+ - JERRYX_ARG_CLAMP - it will try to clamp the number into the range.
+ - JERRYX_ARG_NO_CLAMP - it will throw range error.
+**See also**


missing new line

LaszloLango · 2017-06-13T11:37:21Z

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

+ *         jerry error: the transformer fails.
+ */
+static jerry_value_t
+jerryx_arg_helper_process_double (double *d, /**< [inout] the number to be processed */


[in, out]

LaszloLango

LGTM

akosthekiss

First set of thoughts

akosthekiss · 2017-06-15T18:31:10Z

docs/09.EXT-REFERENCE-ARG.md

@@ -98,6 +98,69 @@ Enum that indicates whether an argument is optional or required.
 - [jerryx_arg_function](#jerryx_arg_function)
 - [jerryx_arg_native_pointer](#jerryx_arg_native_pointer)

+## jerryx_arg_round_t
+
+Enum that indicates the rounding policy which will be choose to transform an integer.


akosthekiss · 2017-06-15T18:35:22Z

docs/09.EXT-REFERENCE-ARG.md

+
+**Summary**
+
+All above jerryx_arg_intX(uintX) are used to Create a validation/transformation step


That (uintX) is misleading as it looks like a function argument. Alternatives: jerryx_arg_[u]intX (or the same but with parentheses instead of brackets), or jerryx_arg_intX and jerryx_arg_uintX (written out in full).

I'd add the word "functions" after _intX (whichever form is used), like: All the above xxxxxx functions

Unnecessary capitalization of Create

akosthekiss · 2017-06-15T18:41:24Z

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

+jerry_value_t jerryx_arg_transform_ ## type  (jerryx_arg_js_iterator_t *js_arg_iter_p, \
+                                              const jerryx_arg_t *c_arg_p); \
+jerry_value_t jerryx_arg_transform_ ## type ## _optional (jerryx_arg_js_iterator_t *js_arg_iter_p, \
+                                                          const jerryx_arg_t *c_arg_p);


indent the body of the define

akosthekiss · 2017-06-15T18:41:33Z

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

+
+#define DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT(type) \
+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type) \
+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type ## _strict)


indent the body of the define

akosthekiss · 2017-06-15T18:47:14Z

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

+JERRYX_STATIC_ASSERT (sizeof (jerryx_arg_int_option_t) <= sizeof (((jerryx_arg_t *) 0)->extra_info),
+                      jerryx_arg_number_options_t_must_fit_into_extra_info);
+
+#undef JERRYX_STATIC_ASSERT


static assertions should usually not go in (public) headers but in sources (as in jerry-core)

akosthekiss · 2017-06-15T18:52:14Z

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

-jerry_value_t jerryx_arg_transform_native_pointer_optional (jerryx_arg_js_iterator_t *js_arg_iter_p,
-                                                            const jerryx_arg_t *c_arg_p);
+
+#define DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type) \


I'm not sure about the DECLARE_ prefix. why not without it?

Here I use the prefix DECLARE_ to indicate it is function declaration.And I use GENERATE_ to indicate it is function impl./defination

akosthekiss · 2017-06-15T18:52:37Z

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

+jerry_value_t jerryx_arg_transform_ ## type ## _optional (jerryx_arg_js_iterator_t *js_arg_iter_p, \
+                                                          const jerryx_arg_t *c_arg_p);
+
+#define DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT(type) \


drop DECLARE_ prefix?

akosthekiss · 2017-06-15T18:53:09Z

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

 jerry_value_t jerryx_arg_transform_ignore (jerryx_arg_js_iterator_t *js_arg_iter_p,
                                           const jerryx_arg_t *c_arg_p);

 /**
+ * The macro used to generate jerryx_arg_xxx for int type.
+ */
+#define GENERATE_JERRYX_ARG_INT(type) \


drop GENERATE_ prefix?

akosthekiss · 2017-06-15T18:55:13Z

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

+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (string)
+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (boolean)
+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL (function)
+DECLARE_JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL (native_pointer)


Macros not undef'd after the last use. Note that this header is included by users of jerry-ext (even if it is an "impl.h"), so whatever is (left) defined here, will clutter the namespace of the user after the include of arg.h

added the #undef in the end of the usage. Thanks for reminding.

akosthekiss · 2017-06-15T19:06:56Z

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

+/**
+ * Indicates the rounding policy which will be choose to transform a integer.
+ */
+typedef enum __attribute__((packed))


The need for an attribute signals that something is non-standard here. Why is this needed? I guess it is to ensure that jerryx_arg_int_option_t fits into extra_info. But that can be solved in other ways. BTW, is it necessary to expose jerryx_arg_int_option_t to the user? Why not have the jerryx_arg_(u)intX functions accept both a jerryx_arg_round_t and a jerryx_arg_clamp_t argument and let those functions encode (pack) the enums into extra_info? E.g., by casting the enum values to integers and shifting-bitmasking them together into a single int value. As a lib user, you could still create mappings easily but without a need for an extra (public) type, e.g.,

jerryx_arg_int8 (&num0, 0, 0, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED) jerryx_arg_int8 (&num0, JERRYX_ARG_ROUND, 0, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED) jerryx_arg_int8 (&num0, 0, JERRYX_ARG_NO_CLAMP, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED) jerryx_arg_int8 (&num0, JERRYX_ARG_ROUND, JERRYX_ARG_NO_CLAMP, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED)

the discussion with @martijnthe is in this thread #1883 (comment)

The main purpose is that

we don't want the signature of the jerryx_arg_[u]intX is too long

jerryx_arg_int_option_t indicates that these are int-specific options from normal number. And people can just pass a empty one to use the default options

we avoid the extra computation (like encode (pack) the enums into extra_info) in jerryx_arg_[u]intX since they are inlined.

Having read the discussion, I'm still not convinced about the need for a public option type.

In the discussion, the option type was planned to contain rounding, clamping, optionality, requiredness - but eventually it just remained with rounding and clamping. Optionality and requiredness are still passed as regular args. So, the functions already take 4 arguments. How much longer is a 5-args signature, when does it count as too long?

The empty initializer is not C but C++, as also mentioned in the discussion. How better is {0} than 0, 0? (Provided that you'd like to use 0-based defaults instead of writing out the enum values explicitly.)

I'm not sure that accessing fields of a struct of packed enums is in any ways different from doing the shifting-masking manually (at the machine code level). But if we want to avoid bit ops, we can still have a non-public struct of two uint8_t fields that contain the enum values. Then, at implementation source code level, we only have to write out casting explicitly (to enum and uint8_t), and the compiler will hide the packing.

jiangzidong · 2017-06-16T05:11:26Z

@akosthekiss updated
About the prefix DECLARE_ ,
it is used to indicate the following part is function declaration instead of the definition. And I use GENERATE_ to indicate it is function impl./defination

akosthekiss · 2017-06-16T09:13:13Z

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

+                                    const jerryx_arg_t *c_arg_p) /**< the native arg */
+{
+  return jerryx_arg_transform_number_strict_common (js_arg_iter_p,
+                                                    c_arg_p->dest);


(style) I see no reason for breaking this function call into multiple lines. we are still within line length limits.

akosthekiss · 2017-06-16T09:13:28Z

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

+                             const jerryx_arg_t *c_arg_p) /**< the native arg */
+{
+  return jerryx_arg_transform_number_common (js_arg_iter_p,
+                                             c_arg_p->dest);


(style) no need to break this call either

akosthekiss · 2017-06-16T09:14:52Z

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

+ * @return the rounded double
+ */
+static double
+jerryx_arg_helper_round (double x) /**< the number need to round */


need to round -> to be rounded ?

akosthekiss · 2017-06-16T09:24:07Z

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

+                                 (jerry_char_t *) "The number is out of range.");
+    }
+  }
+  else


As we are using an enum with potentially multiple values (not necessarily just a boolean yes/no), we should get future-proof. There are three alternatives:

if (x == VALUE1) { ... } else { assert (x == VALUE2); ... }

if (x == VALUE1) { ... } else if (x == VALUE2) { ... } else { assert (false); }

switch (x) { case VALUE1: ... break; case VALUE2: ... break; default: assert (false); }

Perhaps the last one is the best (after fleshing the sketch out).

Does it make sense to perfect the above code after we impl the assert/unreachable/ndebug such things in jerryx?

OK, leave it for now

akosthekiss · 2017-06-16T09:24:52Z

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

+  {
+    *d = floor (*d);
+  }
+  else


same comment here as above, for safely covering all enum values (especially since we really have more than two possible values in this case)

akosthekiss · 2017-06-16T09:54:38Z

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

+{
+  const double t = d < min ? min : d;
+  return t > max ? max : t;
+} /* jerryx_arg_helper_clamp */


is there any reason to have the clamp and round helpers in separate functions? both are called from jerryx_arg_helper_process_double only, and each only once.

akosthekiss · 2017-06-16T10:02:43Z

docs/09.EXT-REFERENCE-ARG.md

+out of the target type.
+
+ - JERRYX_ARG_CLAMP - it will try to clamp the number into the range.
+ - JERRYX_ARG_NO_CLAMP - it will throw range error.


(doc) the descriptions are not word by word identical to the comments in the sources. it might be easier to maintain documentation if they would always be kept in sync. (btw, why is "try to clamp"? can clamping not succeed?)

clamping will not fail. I will change that

akosthekiss · 2017-06-16T10:04:09Z

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

+
+/**
+ * The structure indicates the options used to transform integer argument.
+ * It will be passed into jerryx_arg_t's  extra_info filed.


filed -> field (only if this struct survives)

akosthekiss · 2017-06-16T10:06:35Z

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

+test_validator_int1_handler (const jerry_value_t func_obj_val __attribute__((unused)), /**< function object */
+                            const jerry_value_t this_val __attribute__((unused)), /**< this value */
+                            const jerry_value_t args_p[], /**< arguments list */
+                            const jerry_length_t args_cnt) /**< arguments length */


indentation is off by one space

akosthekiss · 2017-06-16T10:07:46Z

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

+    jerryx_arg_int8 (&num5, opt_ceil, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED),
+    jerryx_arg_int8 (&num6, opt_default, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED),
+    jerryx_arg_int8 (&num7, opt_no_clamp, JERRYX_ARG_COERCE, JERRYX_ARG_REQUIRED),
+


why this empty line within the array?

akosthekiss

One extra request. There is a bad style leftover from old: the body of JERRYX_ARG_TRANSFORM_OPTIONAL was not indented originally. Would it be possible to fix it in this PR?

akosthekiss · 2017-06-17T09:50:11Z

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

+ * Use the macro to define two transform functions for int type.
+ * One is non-strict version and the other is the strict version.
+ */
+#define JERRYX_ARG_TRANSFORM_FUNC_FOR_INT(type, min, max) \


this macro still contains two function implementations which are copy-paste clones of each other except for "_strict". the macro should contain a single function (template) only, and strictness should be an additional macro argument

akosthekiss · 2017-06-17T09:50:49Z

docs/09.EXT-REFERENCE-ARG.md

+
+ Indicates the clamping policy which will be chosen to transform a integer.
+ If the policy is NO_CLAMP, and the number is out of range,
+ then the transformer will throw an range error.


an range -> a range

akosthekiss · 2017-06-17T09:53:36Z

docs/09.EXT-REFERENCE-ARG.md

+
+## jerryx_arg_clamp_t
+
+ Indicates the clamping policy which will be chosen to transform a integer.


a integer -> an integer

akosthekiss · 2017-06-17T09:54:20Z

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

@@ -90,13 +90,52 @@ typedef enum
  JERRYX_ARG_REQUIRED
 } jerryx_arg_optional_t;

+/**
+ * Indicates the rounding policy which will be chosen to transform a integer.


a integer -> an integer

akosthekiss · 2017-06-17T09:54:32Z

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

+} jerryx_arg_round_t;
+
+/**
+ * Indicates the clamping policy which will be chosen to transform a integer.


a integer -> an integer

akosthekiss · 2017-06-17T09:54:49Z

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

+/**
+ * Indicates the clamping policy which will be chosen to transform a integer.
+ * If the policy is NO_CLAMP, and the number is out of range,
+ * then the transformer will throw an range error.


an range -> a range

akosthekiss · 2017-06-17T10:04:02Z

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

+    { \
+      .func = func, \
+      .dest = (void *) dest, \
+      .extra_info = *extra_info_p \


couldn't this be simply .extra_info = *(uintptr_t *) &int_option; without an extra extra_info_p variable?

akosthekiss · 2017-06-17T10:06:07Z

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

+                                 (jerry_char_t *) "The number is out of range.");
+    }
+  }
+  else


OK, leave it for now

jiangzidong · 2017-06-18T06:38:46Z

fixed @akosthekiss

akosthekiss

mostly style issues (strange that style checker did not catch them)

note: the patch will need to be rebased (and conflicts resolved) once the object properties PR lands because of some changes to arg.impl.h .

akosthekiss · 2017-06-19T16:15:03Z

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

+
+/**
+ * Use the macro to define two transform functions for int type.
+ * One is non-strict version and the other is the strict version.


the comment is outdated

akosthekiss · 2017-06-19T16:15:19Z

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

+ */
+#define JERRYX_ARG_TRANSFORM_FUNC_FOR_INT_TEMPLATE(type, suffix, min, max) \
+  jerry_value_t jerryx_arg_transform_ ## type ## suffix (jerryx_arg_js_iterator_t *js_arg_iter_p, \
+                                                          const jerryx_arg_t *c_arg_p) \


indentation is off by one space

akosthekiss · 2017-06-19T16:15:53Z

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

+    } \
+    *(type ## _t *) c_arg_p->dest = (type ## _t) tmp; \
+    return rv; \
+  } \


trailing backslash not needed

akosthekiss · 2017-06-19T16:16:33Z

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

+JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (uint16, 0, UINT16_MAX)
+JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (int16, INT16_MIN, INT16_MAX)
+JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (uint32, 0, UINT32_MAX)
+JERRYX_ARG_TRANSFORM_FUNC_FOR_INT (int32, INT32_MIN , INT32_MAX)


extra spacing (INT32_MIN , INT32_MAX)

akosthekiss · 2017-06-19T16:17:51Z

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

+                      jerryx_arg_round_t round_flag, \
+                      jerryx_arg_clamp_t clamp_flag, \
+                      jerryx_arg_coerce_t coerce_flag, \
+                      jerryx_arg_optional_t opt_flag);


indentation off by one space

akosthekiss · 2017-06-19T16:19:30Z

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

-jerry_value_t jerryx_arg_transform_native_pointer_optional (jerryx_arg_js_iterator_t *js_arg_iter_p,
+
+#define JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type) \
+  jerry_value_t jerryx_arg_transform_ ## type  (jerryx_arg_js_iterator_t *js_arg_iter_p, \


extra spacing (type ()

akosthekiss · 2017-06-19T16:21:09Z

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

+
+#define JERRYX_ARG_TRANSFORM_FUNC_FOR_INT(type, min, max) \
+  JERRYX_ARG_TRANSFORM_FUNC_FOR_INT_TEMPLATE(type, _strict, min, max) \
+  JERRYX_ARG_TRANSFORM_FUNC_FOR_INT_TEMPLATE(type, , min, max)


missing space at TEMPLATE( (between end of macro name and opening parenthesis)

akosthekiss · 2017-06-19T16:22:02Z

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

+
+#define JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT(type) \
+  JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type) \
+  JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL(type ## _strict)


missing space at OPTIONAL(

jiangzidong · 2017-06-20T01:01:36Z

fixed and rebased to the latest master @akosthekiss

akosthekiss · 2017-06-20T16:12:16Z

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

+ */
+#define JERRYX_ARG_TRANSFORM_FUNC_FOR_INT_TEMPLATE(type, suffix, min, max) \
+  jerry_value_t jerryx_arg_transform_ ## type ## suffix (jerryx_arg_js_iterator_t *js_arg_iter_p, \
+                                                        const jerryx_arg_t *c_arg_p) \


indentation still off by one space

akosthekiss · 2017-06-20T16:19:25Z

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

+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (string)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (boolean)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL (function)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL (native_pointer)
 jerry_value_t jerryx_arg_transform_ignore (jerryx_arg_js_iterator_t *js_arg_iter_p,
                                           const jerryx_arg_t *c_arg_p);
 jerry_value_t jerryx_arg_transform_object_props (jerryx_arg_js_iterator_t *js_arg_iter_p,
                                                 const jerryx_arg_t *c_arg_p);
 jerry_value_t jerryx_arg_transform_object_props_optional (jerryx_arg_js_iterator_t *js_arg_iter_p,
                                                          const jerryx_arg_t *c_arg_p);


why haven't these been refactored to use JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL? (that's what I was referring to in my previous review)

I mainly focused on the conflict after rebasing to the integer-check patch, and missed this one.. Fixed

akosthekiss · 2017-06-20T16:20:14Z

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

+  TEST_ASSERT (num7 == 123);
+
+  jerry_release_value (is_ok);
+  validator_int_count ++;


seems like extra spacing

akosthekiss · 2017-06-20T16:20:45Z

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

+  TEST_ASSERT (num11 == -2147483647);
+
+  jerry_release_value (is_ok);
+  validator_int_count ++;


seems like extra spacing

jiangzidong · 2017-06-21T14:21:20Z

What about the current update? @akosthekiss

akosthekiss · 2017-06-21T15:46:54Z

docs/09.EXT-REFERENCE-ARG.md

+
+ - return value - the created `jerryx_arg_t` instance.
+ - `dest` - pointer to the `int32_t` where the result should be stored.
+ - `round_flag` - the roudning policy.


roudning -> rounding

akosthekiss · 2017-06-21T15:53:14Z

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

+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (number)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (string)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (boolean)
+JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT (object_props)


This is not right. JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL_AND_STRICT will declare

jerryx_arg_transform_object_props jerryx_arg_transform_object_props_optional jerryx_arg_transform_object_props_strict jerryx_arg_transform_object_props_strict_optional

but the latter two do not exist. JERRYX_ARG_TRANSFORM_FUNC_WITH_OPTIONAL should be used instead.

My bad. Stupid mistake...

JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

akosthekiss

LGTM

jiangzidong added the jerry-ext Related to the jerry-ext library label Jun 7, 2017

martijnthe reviewed Jun 7, 2017

View reviewed changes

LaszloLango requested changes Jun 7, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 51da045 to aa3b9c9 Compare June 8, 2017 04:39

jiangzidong force-pushed the arg-int branch from aa3b9c9 to 8096670 Compare June 8, 2017 10:11

martijnthe reviewed Jun 8, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 1f1ab7d to 2d434c0 Compare June 8, 2017 13:19

LaszloLango requested changes Jun 13, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 2d434c0 to 6b18eb9 Compare June 13, 2017 10:18

LaszloLango requested changes Jun 13, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 6b18eb9 to 1120d04 Compare June 13, 2017 12:04

LaszloLango approved these changes Jun 15, 2017

View reviewed changes

akosthekiss added the enhancement An improvement label Jun 15, 2017

akosthekiss requested changes Jun 15, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 1120d04 to 8b54ecc Compare June 16, 2017 05:09

akosthekiss requested changes Jun 16, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 8b54ecc to e4b5e86 Compare June 17, 2017 01:58

akosthekiss requested changes Jun 17, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from e4b5e86 to 8923a87 Compare June 18, 2017 06:38

akosthekiss requested changes Jun 19, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from 8923a87 to a49ef8e Compare June 20, 2017 01:01

akosthekiss requested changes Jun 20, 2017

View reviewed changes

jiangzidong force-pushed the arg-int branch from a49ef8e to e639f6e Compare June 21, 2017 00:41

akosthekiss requested changes Jun 21, 2017

View reviewed changes

add transform functions for integer in jerryx/arg

7efe558

JerryScript-DCO-1.0-Signed-off-by: Zidong Jiang [email protected]

jiangzidong force-pushed the arg-int branch from e639f6e to 7efe558 Compare June 21, 2017 16:03

akosthekiss approved these changes Jun 21, 2017

View reviewed changes

jiangzidong merged commit b153475 into jerryscript-project:master Jun 22, 2017

jiangzidong deleted the arg-int branch June 22, 2017 08:30


		Summary

		All above jerryx_arg_intX(uintX) are used to Create a validation/transformation step


		## jerryx_arg_clamp_t

		Indicates the clamping policy which will be chosen to transform a integer.

add transform functions for integer in jerryx/arg #1883

add transform functions for integer in jerryx/arg #1883

Uh oh!

Conversation

jiangzidong commented Jun 7, 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!

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

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!

jiangzidong commented Jun 8, 2017

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jiangzidong commented Jun 13, 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!

jiangzidong commented Jun 13, 2017

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!

akosthekiss 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

jiangzidong Jun 8, 2017 •

edited

Loading

jiangzidong commented Jun 8, 2017 •

edited

Loading