n-api: add optional string length parameter #15343

sampsongao · 2017-09-11T14:31:51Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

jasongin · 2017-09-11T15:52:42Z

We need new test cases that verify the behavior of each of these APIs with lengths other than -1. For each API a test should pass in a longer string value and then check that the result is truncated according to the length.

mhdawson · 2017-09-15T12:59:36Z

CI run: https://ci.nodejs.org/job/node-test-pull-request/10091/

mhdawson · 2017-09-15T13:41:43Z

There seem to be ci failures on BSD:

https://ci.nodejs.org/job/node-test-commit-freebsd/11584/nodes=freebsd10-64/console

gypgmake[2]: Entering directory '/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/addons-napi/test_constructor/build'
 info spawn  CC(target) Release/obj.target/test_constructor/test_constructor.o
 gmake
gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build', '--jobs', 2 ]
  CC(target) Release/obj.target/test_constructor_name/test_constructor_name.o
../test_constructor.c:80:3: error: non-void function 'Init' should return a value [-Wreturn-type]
  NAPI_CALL_RETURN_VOID(env, napi_define_class(env, "MyObject", -1, New,
  ^
../../common.h:54:3: note: expanded from macro 'NAPI_CALL_RETURN_VOID'
  NAPI_CALL_BASE(env, the_call, NAPI_RETVAL_NOTHING)
  ^
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;

mhdawson · 2017-09-15T14:32:47Z

New CI run: https://ci.nodejs.org/job/node-test-pull-request/10094/

mhdawson · 2017-09-15T14:34:34Z

doc/api/n-api.md

@@ -552,11 +552,18 @@ thrown to immediately terminate the process.
 added: v8.2.0
 -->
 ```C
-NAPI_NO_RETURN void napi_fatal_error(const char* location, const char* message);
+NAPI_EXTERN NAPI_NO_RETURN void napi_fatal_error(const char* location,
+                                                 size_t location_len,


Should NAPI_EXTERN be added here ?

mhdawson · 2017-09-15T14:37:18Z

src/node_api.cc

+  char* location_string = const_cast<char*>(location);
+  char* message_string = const_cast<char*>(message);
+  if (location_len != -1) {
+    location_string = (char*) malloc(location_len * sizeof(char) + 1);


I thought we were going to use stack allocation, won't the malloc's result in a memory leak ?

@sampsongao pointed out that the process will terminate when we call napi_fatal_error so it should be ok.

mhdawson

LGTM

mhdawson · 2017-09-15T16:11:30Z

Still failures on freebsd


Building addon /usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/addons/errno-exception/
../test_function.c:33:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_create_function(
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:38:24: warning: incompatible pointer types passing 'void (napi_env, napi_callback_info)' to parameter of type 'napi_callback' (aka 'napi_value (*)(napi_env, napi_callback_info)') [-Wincompatible-pointer-types]
      env, "Name", -1, TestFunctionName, NULL, &fn2));
                       ^~~~~~~~~~~~~~~~
../../common.h:50:23: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
                      ^
../../common.h:42:10: note: expanded from macro 'NAPI_CALL_BASE'
    if ((the_call) != napi_ok) {                                         \
         ^
/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/src/node_api.h:160:60: note: passing argument to parameter 'cb' here
                                             napi_callback cb,
                                                           ^
../test_function.c:37:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_create_function(
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:42:29: warning: incompatible pointer types passing 'void (napi_env, napi_callback_info)' to parameter of type 'napi_callback' (aka 'napi_value (*)(napi_env, napi_callback_info)') [-Wincompatible-pointer-types]
      env, "Name_extra", 5, TestFunctionName, NULL, &fn3));
                            ^~~~~~~~~~~~~~~~
../../common.h:50:23: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
                      ^
../../common.h:42:10: note: expanded from macro 'NAPI_CALL_BASE'
    if ((the_call) != napi_ok) {                                         \
         ^
/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/src/node_api.h:160:60: note: passing argument to parameter 'cb' here
                                             napi_callback cb,
                                                           ^
../test_function.c:41:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_create_function(
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:44:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_set_named_property(env, exports, "TestCall", fn1));
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:45:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_set_named_property(env, exports, "TestName", fn2));
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:46:3: error: void function 'Init' should not return a value [-Wreturn-type]
  NAPI_CALL(env, napi_set_named_property(env, exports, "TestNameShort", fn3));
  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../common.h:50:3: note: expanded from macro 'NAPI_CALL'
  NAPI_CALL_BASE(env, the_call, NULL)
  ^                             ~~~~
../../common.h:44:7: note: expanded from macro 'NAPI_CALL_BASE'
      return ret_val;                                                    \
      ^
../test_function.c:49:35: warning: incompatible pointer types initializing 'napi_addon_register_func' (aka 'napi_value (*)(napi_env, napi_value)') with an expression of type 'void (napi_env, napi_value, napi_value, void *)' [-Wincompatible-pointer-types]
NAPI_MODULE(NODE_GYP_MODULE_NAME, Init)
                                  ^~~~
/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/src/node_api.h:101:26: note: expanded from macro 'NAPI_MODULE'
  NAPI_MODULE_X(modname, regfunc, NULL, 0)
                         ^
/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/src/node_api.h:90:7: note: expanded from macro 'NAPI_MODULE_X'
      regfunc,                                                        \
      ^
3 warnings and 6 errors generated.

mhdawson · 2017-09-18T14:35:19Z

new CI run: https://ci.nodejs.org/job/node-test-pull-request/10122/

mhdawson · 2017-09-18T15:26:14Z

CI looks good except for linter failure. I believe that was a problem either in CI or in master as the previous few linter jobs failed in the same way.

mhdawson · 2017-09-18T15:30:31Z

Seems like lint issue was not ci related:

make: *** No rule to make target 'lin'.  Stop.
user1@user1-lbuntu:~/newpull/land/node$ make lint
Running JS linter...
./node tools/eslint/bin/eslint.js --cache --rulesdir=tools/eslint-rules --ext=.js,.mjs,.md \
  benchmark doc lib test tools

/home/user1/newpull/land/node/test/addons-napi/test_constructor/test2.js
  8:1  error  'assert.equal' is restricted from being used. Use assert.strictEqual() rather than assert.equal()  no-restricted-properties

✖ 1 problem (1 error, 0 warnings)

Makefile:886: recipe for target 'jslint' failed
make[1]: *** [jslint] Error 1
Running C++ linter...
src/node_api.cc:942:  Using C-style cast.  Use reinterpret_cast<char*>(...) instead  [readability/casting] [4]
src/node_api.cc:947:  Using C-style cast.  Use reinterpret_cast<char*>(...) instead  [readability/casting] [4]
test/addons-napi/7_factory_wrap/myobject.cc:25:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
test/addons-napi/6_object_wrap/myobject.cc:26:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
test/addons-napi/8_passing_wrapped/myobject.cc:20:  Lines should be <= 80 characters long  [whitespace/line_length] [2]
Total errors found: 5
Makefile:921: recipe for target 'cpplint' failed
make[1]: *** [cpplint] Error 1
Makefile:927: recipe for target 'lint' failed
make: *** [lint] Error 2

mhdawson · 2017-09-18T17:46:54Z

Another CI run: https://ci.nodejs.org/job/node-test-pull-request/10126/

mhdawson · 2017-09-18T21:43:01Z

Arm failure was: nodejs/build#884

mhdawson · 2017-09-18T21:43:40Z

CI was good going to land.

PR-URL: #15343 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

mhdawson · 2017-09-18T22:07:51Z

Landed as 1976654

PR-URL: #15343 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

PR-URL: nodejs/node#15343 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

PR-URL: nodejs#15343 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

Backport-PR-URL: #19447 PR-URL: #15343 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Sep 11, 2017

sampsongao mentioned this pull request Sep 11, 2017

n-api: Add optional length to functions accepting const char * #15108

Closed

Sampson Gao added 6 commits September 14, 2017 23:06

n-api: add optional string length parameters

07e9df4

napi: update doc for additional string length parameter

53c4e81

n-api: fix build errors

7a7dc8b

test: update test for additional string length parameter

ef1af4a

Handle non-zero terminated string in napi_fatal_error

8762a3c

Add tests for non-zero termianted string input

c0b202c

sampsongao force-pushed the optional_str_param branch from 5f8d256 to d9cab83 Compare September 15, 2017 04:36

Fix errors after rebasing

cf0f4fa

sampsongao force-pushed the optional_str_param branch from d9cab83 to cf0f4fa Compare September 15, 2017 04:40

Fix CI run fail

90ef051

mhdawson reviewed Sep 15, 2017

View reviewed changes

mhdawson approved these changes Sep 15, 2017

View reviewed changes

jasnell approved these changes Sep 15, 2017

View reviewed changes

Fix remaining rebasing difference

b0175ae

sampsongao force-pushed the optional_str_param branch from 225289d to b0175ae Compare September 15, 2017 23:05

fhinkel approved these changes Sep 16, 2017

View reviewed changes

sampsongao force-pushed the optional_str_param branch from 6bd811d to cffe74e Compare September 18, 2017 15:38

Fix linter error

8e7bf47

sampsongao force-pushed the optional_str_param branch from cffe74e to 8e7bf47 Compare September 18, 2017 17:20

mhdawson closed this Sep 18, 2017

jasnell mentioned this pull request Sep 20, 2017

Node.js 8.6.0 Release Proposal #15509

Merged

MylesBorins added the dont-land-on-v6.x label Oct 17, 2017

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add optional string length parameter #15343

n-api: add optional string length parameter #15343

sampsongao commented Sep 11, 2017

jasongin commented Sep 11, 2017

mhdawson commented Sep 15, 2017

mhdawson commented Sep 15, 2017 •

edited

mhdawson commented Sep 15, 2017

mhdawson Sep 15, 2017

mhdawson Sep 15, 2017

mhdawson Sep 15, 2017 •

edited

mhdawson left a comment

mhdawson commented Sep 15, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

n-api: add optional string length parameter #15343

n-api: add optional string length parameter #15343

Conversation

sampsongao commented Sep 11, 2017

Checklist

Affected core subsystem(s)

jasongin commented Sep 11, 2017

mhdawson commented Sep 15, 2017

mhdawson commented Sep 15, 2017 • edited

mhdawson commented Sep 15, 2017

mhdawson Sep 15, 2017

Choose a reason for hiding this comment

mhdawson Sep 15, 2017

Choose a reason for hiding this comment

mhdawson Sep 15, 2017 • edited

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented Sep 15, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 18, 2017

mhdawson commented Sep 15, 2017 •

edited

mhdawson Sep 15, 2017 •

edited