Skip to content

Conversation

sampsongao
Copy link

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)

@nodejs-github-bot 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
@jasongin
Copy link
Member

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
Copy link
Member

@mhdawson
Copy link
Member

mhdawson commented Sep 15, 2017

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
Copy link
Member

```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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should NAPI_EXTERN be added here ?

src/node_api.cc Outdated
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mhdawson mhdawson Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mhdawson
Copy link
Member

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
Copy link
Member

@mhdawson
Copy link
Member

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
Copy link
Member

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
Copy link
Member

@mhdawson
Copy link
Member

Arm failure was: nodejs/build#884

@mhdawson
Copy link
Member

CI was good going to land.

mhdawson pushed a commit that referenced this pull request Sep 18, 2017
PR-URL: #15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@mhdawson
Copy link
Member

Landed as 1976654

@mhdawson mhdawson closed this Sep 18, 2017
jasnell pushed a commit that referenced this pull request Sep 20, 2017
PR-URL: #15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
PR-URL: nodejs/node#15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Qard pushed a commit to Qard/ayo that referenced this pull request Sep 21, 2017
PR-URL: nodejs/node#15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2018
PR-URL: nodejs#15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
MylesBorins pushed a commit that referenced this pull request Apr 16, 2018
Backport-PR-URL: #19447
PR-URL: #15343
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants