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.

Sign up for GitHub

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

fix: call base basic finalizer if none defined #1574

Merged
merged 2 commits into from
Sep 5, 2024
+4 −0
Merged

fix: call base basic finalizer if none defined #1574

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
427ef4d
fix: call base basic finalizer if none defined
KevinEady Sep 4, 2024
72655c6
discard `env` value
KevinEady Sep 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions napi-inl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5023,6 +5023,11 @@ inline void ObjectWrap<T>::FinalizeCallback(node_api_nogc_env env,
#endif

instance->Finalize(Napi::BasicEnv(env));
} else {
Copy link
Member

Choose a reason for hiding this comment

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

I see how this fixes the existing, not used error, but does it also fix running some finalization that should have run before but would not have, or now call the method just to fix the not used error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

but does it also fix running some finalization that should have run before but would not have

This is not the case. If the user defined their own MyClass::Finalize(BasicEnv), the constexpr (details::HasBasicFinalizer<T>::value) would be true, and then call instance->Finalize(Napi::BasicEnv(env)).

... now call the method just to fix the not used error

This is the case, so it is adding an inefficiency (perhaps the compiler optimizes it out)

Copy link
Member

Choose a reason for hiding this comment

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

My questions is to make sure the additional invocation is desired, if so good, sounds like we found a potential bug as well. If not I think we can just add a (void) env; early in the function to mark it as used to address the warning.

Copy link
Member

@NickNaso NickNaso Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm trying the same solution you proposed adding (void) env; on my local machine.

Copy link
Member

Choose a reason for hiding this comment

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

it worked well on my local machine.

// The child class may override Finalize but _not_ with a BasicEnv. Calling
// via `instance->Finalize()` would then give a compile error. Explicitly
// use the ObjectWrap<T>'s Finalize, which has BasicEnv override.
instance->ObjectWrap<T>::Finalize(Napi::BasicEnv(env));
}

// If class overrides the (extended) finalizer, either schedule it or
Expand Down