Failures in CI across platforms after nogc env update

[mhdawson]

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

../../napi-inl.h:5011:63: error: parameter ‘env’ set but not used [-Werror=unused-but-set-parameter]
 inline void ObjectWrap<T>::FinalizeCallback(node_api_nogc_env env,

[mhdawson]

@KevinEady is that something you have some cycles to look at ?

[mhdawson]

It looks like env is unused with the default defines, should just need to adjust the parameter definition, looking at it.

[KevinEady]

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:

node-addon-api/napi-inl.h

Lines 4924 to 4932 in bf49519

	template <typename T>
	inline void ObjectWrap<T>::FinalizeCallback(napi_env env,
	void* data,
	void* /*hint*/) {
	HandleScope scope(env);
	T* instance = static_cast<T*>(data);
	instance->Finalize(Napi::Env(env));
	delete instance;
	}

node-addon-api/napi-inl.h

Lines 4837 to 4838 in bf49519

	template <typename T>
	inline void ObjectWrap<T>::Finalize(Napi::Env /*env*/) {}

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.

[KevinEady]

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::PostFinalizeCallback if there is a delayed finalizer.

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.

[NickNaso]

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
...

[mhdawson]

@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.

[mhdawson]

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.