Closed mhdawson closed 1 week ago
@KevinEady is that something you have some cycles to look at ?
It looks like env
is unused with the default defines, should just need to adjust the parameter definition, looking at it.
It looks like
env
is unused with the default defines, should just need to adjust the parameter definition, looking at it.
It's not as simple because of the constexpr
checks: we only need to use env
if the base class defines certain methods.
The test fails with:
In file included from ../../napi.h:3257,
from ../addon_data.cc:3:
../../napi-inl.h: In instantiation of ‘static void Napi::ObjectWrap<T>::FinalizeCallback(node_api_nogc_env, void*, void*) [with T = Addon::VerboseIndicator; node_api_nogc_env = napi_env__*]’:
... and this is because Addon::VerboseIndicator
does not have a Finalize
. Since there's no VerboseIndicator::Finalize
, the ObjectWrap::FinalizeCallback
doesn't use the env
parameter.
Looking at the previous implementation: the ObjectWrap
base class defines an empty Finalize()
to call with env
:
https://github.com/nodejs/node-addon-api/blob/bf49519a4ce08ee5320327c9a0199cd89d5b87b3/napi-inl.h#L4924-L4932
https://github.com/nodejs/node-addon-api/blob/bf49519a4ce08ee5320327c9a0199cd89d5b87b3/napi-inl.h#L4837-L4838
Should I do something similar...? Seems odd to need to do this just to get rid of the warning... but I could imagine some projects using the same warnings-as-error configuration for this unused-but-set-parameter
.
Ah, okay ... I had to use the constexpr
approach because the delete instance;
either happens immediately (if constexpr for "has extended finalizer" condition fails) or gets delayed to the ObjectWrap
Doing something similar to the old approach (ie. defining a no-op, non-basic Finalize
) would not work, since the delete instance
would then be delayed.
Soooo not sure exactly how to address.
Could be the following code a possible solution:
...
template <typename T>
#ifndef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
inline void ObjectWrap<T>::FinalizeCallback(node_api_nogc_env env,
void* data,
void* /*hint*/) {
#else
inline void ObjectWrap<T>::FinalizeCallback(napi_env /*env*/,
void* data,
void* /*hint*/) {
#endif
...
@NickNaso I was thinking about something similar to what you suggested except:
inline void ObjectWrap<T>::FinalizeCallback(
#ifndef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER
node_api_nogc_env env,
#else
node_api_nogc_env /*env*/,
#endif
void* data,
void* /*hint*/) {
EDIT: but looking at the two I think I prefer the one that @NickNaso suggested.
But looking at it I don't think that will work because of the constexpr as the use of env is not just based on the define.
There have been some failures since I landed the PR
https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=rhel8-x64/9278/console
https://ci.nodejs.org/job/node-test-node-addon-api-new/nodes=debian10-x64/9278/console
Looks like a warning about an unused parameter