Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

napi: Remove napi_set_function_name since not all engines can rename functions #142

Merged

Conversation

boingoing
Copy link

Add an optional name parameter to napi_create_function.

Also change V8LocalValueFromJsPropertyName to V8LocalStringFromJsPropertyName instead since these are always string values.

Add an optional name parameter to napi_create_function.

Also change V8LocalValueFromJsPropertyName to V8LocalStringFromJsPropertyName instead since these are always string values.
mhdawson

This comment was marked as off-topic.

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

Seems to have failed CI, gyp info spawn args [ 'BUILDTYPE=Release', '-C', 'build' ]
make[2]: Entering directory `/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/test/addons/02_object_factory/build'
  CXX(target) Release/obj.target/addon/addon.o
../binding.cc: In function 'void CreateFunction(napi_env, napi_callback_info)':
../binding.cc:26:48: error: 'napi_set_function_name' was not declared in this scope
   status = napi_set_function_name(env, fn, name);
                                                ^
../binding.cc: In function 'void Init(napi_env, napi_value, napi_value)':
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::getter' [-Wmissing-field-initializers]
   napi_property_descriptor desc = { "exports", CreateFunction };
                                                               ^
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::setter' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::value' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::attributes' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::data' [-Wmissing-field-initializers]
make[2]: *** [Release/obj.target/binding/binding.o] Error 1
make[2]: Leaving directory `/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/test/addons-abi/5_function_factory/build'
gyp ERR! build error 
gyp ERR! stack Error: `make` failed with exit code: 2
gyp ERR! stack     at ChildProcess.onExit (/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/deps/npm/node_modules/node-gyp/lib/build.js:276:23)
gyp ERR! stack     at emitTwo (events.js:106:13)
gyp ERR! stack     at ChildProcess.emit (events.js:192:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:215:12)
gyp ERR! System Linux 3.13.0-71-generic
gyp ERR! command "/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/out/Release/node" "/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/deps/npm/node_modules/node-gyp/bin/node-gyp" "rebuild" "--python=/usr/bin/python" "--directory=/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/test/addons-abi/5_function_factory/" "--nodedir=/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64"
gyp ERR! cwd /home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/test/addons-abi/5_function_factory
gyp ERR! node -v v8.0.0-pre
gyp ERR! node-gyp -v v3.5.0
gyp ERR! not ok 
make[1]: *** [test/addons-abi/.buildstamp] Error 1
make[1]: *** Waiting for unfinished jobs....

@mhdawson
Copy link
Member

mhdawson commented Mar 14, 2017

Seems like there were places the function was still being used as the error was:

make[2]: Entering directory `/home/iojs/build/workspace/node-test-commit-linux-abi/nodes/ubuntu1404-64/test/addons/02_object_factory/build'
CXX(target) Release/obj.target/addon/addon.o
../binding.cc: In function 'void CreateFunction(napi_env, napi_callback_info)':
../binding.cc:26:48: error: 'napi_set_function_name' was not declared in this scope
status = napi_set_function_name(env, fn, name);
^

While looking at that can you also look at these warnings:

../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::getter' [-Wmissing-field-initializers]
   napi_property_descriptor desc = { "exports", CreateFunction };
                                                               ^
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::setter' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::value' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::attributes' [-Wmissing-field-initializers]
../binding.cc:35:63: warning: missing initializer for member 'napi_property_descriptor::data' [-Wmissing-field-initializers]

We need to make sure we run make test on all PRs and validate that they pass.

@boingoing
Copy link
Author

Missed this one. I fixed the call to napi_create_function but forgot to remove the call to napi_set_function_name.

I thought the warnings reported for missing initializers were allowed by spec? Any missing initializers are converted to 0 as long as there was an initializer, I thought. It's just one line, so I went ahead and removed the warnings but I remember seeing this elsewhere.

I've been dev'ing this on Windows and found that vcbuild.cmd x64 debug test doesn't pass all tests. Should it?

@boingoing
Copy link
Author

Fixes #78, by the way.

@mhdawson
Copy link
Member

I know all the tests should pass without the debug, is that clean ? If so then I'd check the current Node master to see if the debug equivalent passes all tests or not.

mhdawson

This comment was marked as off-topic.

jasongin

This comment was marked as off-topic.

jasongin

This comment was marked as off-topic.

@boingoing boingoing merged commit 45f84c7 into nodejs:api-prototype-8.x Mar 15, 2017
@mhdawson
Copy link
Member

Think this broke the tests, see #148

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants