Initial refactoring of the code to match the latest API. #1212

slaff · 2016-07-17T20:14:25Z

Fixes issue #1211.

LaszloLango · 2016-07-18T06:50:08Z

@slaff, thanks for fixing the ESP8266 port. Please fix your sing-off in the commit message. Check travis logs for details:

Signed-off-by message is incorrect. The following line should be at the end of the 85b5195 commit's message: 'JerryScript-DCO-1.0-Signed-off-by: Slavey Karadzhov [email protected]'.

The commit message should mention ESP8266 and the related issue number too.

LaszloLango · 2016-07-18T06:56:07Z

targets/esp8266/source/jerry_extapi.c

-                  const jerry_api_length_t   args_cnt)
+static jerry_value_t \
+NAME ## _handler (const jerry_value_t  function_obj_p __UNSED__, \
+                  const jerry_value_t  this_p __UNSED__, \


Please remove the _p suffix. They are not pointers anymore. ex.: func_obj_val and this_val

slaff · 2016-07-18T08:26:16Z

@LaszloLango is there an automated script that fixes the coding style issues or at least reports them?

LaszloLango · 2016-07-18T09:41:17Z

@slaff, make check-vera checks the coding style, but unfortunately it does not check the target folder, because it is allowed to use different style there. The only requirement is to use consistence style. For example if you put a space before parenthesis in function calls, then put it everywhere.

To follow Jerry style you can call vera++ manually to your source folder:
./tools/check-vera.sh targets/esp8266/source/

LaszloLango · 2016-07-19T07:31:07Z

targets/esp8266/source/jerry_run.c

-  jerry_api_release_value (&res);
+
+  if (jerry_value_has_error_flag (res)) {
+	  ret_code = -3;


wrong indentation

LaszloLango · 2016-07-19T07:38:09Z

I gave a few more comments. Only minor style issues. I think it close to be ready to merge. Nice work. Thanks again for fixing this port.

slaff · 2016-07-19T07:59:46Z

@LaszloLango I've made the recommended coding style changes. Should I squash the commits or you will squash them during merge?

LaszloLango · 2016-07-19T08:08:05Z

targets/esp8266/source/jerry_extapi.c

-                && jerry_api_is_function (reg_func_p)
-                && jerry_api_is_constructor (reg_func_p)))
+  if (!(jerry_value_is_function (reg_func_val)
+		&& jerry_value_is_constructor (reg_func_val)))


tab character :( It breaks the indentation and make the code ugly

LaszloLango · 2016-07-19T08:09:45Z

LGTM after all of the tab characters are removed. Yes, please squash the commits and rebase to the current master.

slaff · 2016-07-19T08:30:18Z

@LaszloLango I've added the tab fixes, rebased on current master, and squashed the commits.

LaszloLango · 2016-07-19T08:53:57Z

@slaff, thanks. LGTM

zherczeg · 2016-07-19T11:04:49Z

targets/esp8266/source/jerry_extapi.c

+        jerry_size_t size, maxsize;
+        size = jerry_get_string_size (args_p[0]);
+        maxsize  = MIN(size, 126);
+        jerry_string_to_char_buffer (args_p[cc],


I hope you are aware that if the buffer is too small, no bytes are copied.

the line above should do the trick :)

Do the trick? I mean if string length > 126, than MIN() returns with 126, and no bytes will be copied since buffer is too small.

I guess the initial code was to allow the native "print" function to print string(s) with max length of 128 characters. What do you suggest to be changed here?

You can allocate a large enough buffer with malloc, or you can just print that string is too long.

@zherczeg, the current code will load maximum 126 bytes into a 128 bytes buffer. How could it be too small?

BTW, I agree that it would look nicer, if we always allocate the buffer with the appropriate size. I'm just trying to say here the current code is valid and should work. :)

zherczeg · 2016-07-20T09:06:36Z

targets/esp8266/source/jerry_extapi.c

+        jerry_string_to_char_buffer (args_p[cc],
+                                     (jerry_char_t *) buffer,
+                                     maxsize);
+        *(buffer + size) = 0;


No the code is unfortunately not correct. This expression can be a buffer overrun, and a byte on the stack will be replaced with 0. But even if we change size to maxsize, a random character sequence will be printed for strings > 126 bytes.

LaszloLango · 2016-07-21T07:33:41Z

@slaff, please update PR as @zherczeg suggested. I think after that change we can land your PR.

slaff · 2016-07-21T18:42:52Z

@LaszloLango @zherczeg When you have time take a look at the updated native print function. In the worst cases I can remove the native "print" function because the built-in JavaScript function works just fine.

LaszloLango · 2016-07-22T06:37:41Z

@slaff, please fix your Sign-off in the commit message

LaszloLango · 2016-07-22T06:40:04Z

targets/esp8266/source/jerry_extapi.c

-        maxsize  = MIN(size, 126);
+        char *buffer;
+        jerry_size_t size = jerry_get_string_size (args_p[0]);
+        buffer = (char *) malloc(size+1);


Minor style issue: missing spaces
malloc (size + 1);

robertsipka · 2016-07-22T08:55:48Z

targets/esp8266/source/jerry_run.c

+  if (jerry_value_has_error_flag (res)) {
+    ret_code = -3;
+  }
+
  free (val_args);


Is this free call still needed?

Thanks for noticing this. It is removed now.

LaszloLango · 2016-07-22T10:03:09Z

LGTM

zherczeg · 2016-07-27T11:24:37Z

LGTM. Please squash the commits and rebase it to the current master.

Fixes issue jerryscript-project#1211. JerryScript-DCO-1.0-Signed-off-by: Slavey Karadzhov [email protected]

slaff · 2016-07-27T11:37:21Z

LGTM. Please squash the commits and rebase it to the current master.

@zherczeg Done

zherczeg · 2016-07-27T12:20:34Z

Landed.

LaszloLango added the jerry-port Related to the port API or the default port implementation label Jul 18, 2016

LaszloLango reviewed Jul 18, 2016
View reviewed changes

slaff force-pushed the master branch from 85b5195 to d3ea6a7 Compare July 18, 2016 08:25

slaff force-pushed the master branch from d3ea6a7 to 3cee0e5 Compare July 18, 2016 19:23

LaszloLango reviewed Jul 19, 2016
View reviewed changes

slaff force-pushed the master branch from 6176db8 to 83e54ff Compare July 19, 2016 08:01

LaszloLango reviewed Jul 19, 2016
View reviewed changes

slaff force-pushed the master branch from 83e54ff to 00ed6f7 Compare July 19, 2016 08:28

zherczeg reviewed Jul 19, 2016
View reviewed changes

slaff force-pushed the master branch 3 times, most recently from fe4f8e3 to 2c83fd7 Compare July 19, 2016 14:52

zherczeg reviewed Jul 20, 2016
View reviewed changes

LaszloLango reviewed Jul 22, 2016
View reviewed changes

slaff force-pushed the master branch from 858d120 to 4b83e30 Compare July 22, 2016 07:24

robertsipka reviewed Jul 22, 2016
View reviewed changes

slaff force-pushed the master branch from 4b83e30 to 35fd50e Compare July 22, 2016 10:01

Refactoring of the ESP8266 target code to match the latest API.

23db8b5

Fixes issue jerryscript-project#1211. JerryScript-DCO-1.0-Signed-off-by: Slavey Karadzhov [email protected]

slaff force-pushed the master branch from 35fd50e to 23db8b5 Compare July 27, 2016 11:36

zherczeg merged commit 23db8b5 into jerryscript-project:master Jul 27, 2016

tilmannOSG mentioned this pull request Jul 27, 2016

Building JerryScript for ESP8266 fails with: Missing jerry.h #1211

Closed

zherczeg mentioned this pull request Mar 27, 2018

Nominating Robert Sipka (robertsipka) to become a reviewer #2254

Closed

Initial refactoring of the code to match the latest API. #1212

Initial refactoring of the code to match the latest API. #1212

Uh oh!

Conversation

slaff commented Jul 17, 2016

Uh oh!

LaszloLango commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slaff commented Jul 18, 2016

Uh oh!

LaszloLango commented Jul 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented Jul 19, 2016

Uh oh!

slaff commented Jul 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LaszloLango commented Jul 19, 2016

Uh oh!

slaff commented Jul 19, 2016

Uh oh!

LaszloLango commented Jul 19, 2016

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 commented Jul 21, 2016

Uh oh!

slaff commented Jul 21, 2016

Uh oh!

LaszloLango commented Jul 22, 2016

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 commented Jul 22, 2016

Uh oh!

zherczeg commented Jul 27, 2016

Uh oh!

slaff commented Jul 27, 2016

Uh oh!

zherczeg commented Jul 27, 2016

Uh oh!

Uh oh!