feat: add support for requiring basic finalizers #1568
Conversation
KevinEady
force-pushed
the
add-require-basic-finalizers
branch
from
4603869 to
e21c0f6
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1568 +/- ##
=======================================
Coverage 64.40% 64.40%
=======================================
Files 3 3
Lines 2003 2003
Branches 693 693
=======================================
Hits 1290 1290
Misses 146 146
Partials 567 567 ☔ View full report in Codecov by Sentry. |
KevinEady
force-pushed
the
add-require-basic-finalizers
branch
from
e21c0f6 to
7c91a10
Compare
|
When trying to use a non-basic finalizer with this directive defined, you'll get an error like: (error taken from the test created in this PR) |
doc/finalization.md
Outdated
| @@ -8,7 +8,9 @@ provide more efficient memory management, optimizations, improved execution, or | |||
| other benefits. | |||
|
|
|||
| In general, it is best to use basic finalizers whenever possible (eg. when | |||
| access to JavaScript is _not_ needed). | |||
| access to JavaScript is _not_ needed). To ensure that all finalizers are basic | |||
| finalizers at compile-time, define the `NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS` | |||
There was a problem hiding this comment.
I am wondering if we should add a "best practice" target that enables all possible checks in https://github.com/nodejs/node-addon-api/blob/main/node_addon_api.gyp.
doc/finalization.md
Outdated
| @@ -8,7 +8,9 @@ provide more efficient memory management, optimizations, improved execution, or | |||
| other benefits. | |||
|
|
|||
| In general, it is best to use basic finalizers whenever possible (eg. when | |||
| access to JavaScript is _not_ needed). | |||
| access to JavaScript is _not_ needed). To ensure that all finalizers are basic | |||
There was a problem hiding this comment.
Let's make this passive voice: The NODE_ADDON_API_REQUIRE_BASIC_FINALIZERS preprocessor directive can be defined to ensure that all finalizers are basic.
There was a problem hiding this comment.
Addressed in 23b57cc
doc/setup.md
Outdated
| @@ -90,3 +90,13 @@ provide feedback to the user of the runtime error, as it is impossible to pass | |||
| the error to JavaScript when the environment is terminating. In order to bypass | |||
| this behavior such that the Node process will not terminate, define the | |||
There was a problem hiding this comment.
Here too: "The ... can be defined in order to bypass ... "
There was a problem hiding this comment.
Addressed in 23b57cc
napi-inl.h
Outdated
| @@ -205,14 +205,19 @@ struct FinalizeData { | |||
| }); | |||
| } | |||
|
|
|||
| #ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | |||
| #if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) | |||
There was a problem hiding this comment.
Is this change needed?
There was a problem hiding this comment.
Whoops, this was a leftover of a different implementation. Addressed in 23b57cc
napi-inl.h
Outdated
| @@ -235,14 +240,19 @@ struct FinalizeData { | |||
| }); | |||
| } | |||
|
|
|||
| #ifdef NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER | |||
| #if defined(NODE_API_EXPERIMENTAL_HAS_POST_FINALIZER) | |||
There was a problem hiding this comment.
Addressed in 23b57cc
- Use passive voice in existing and new docs - Revert unnecessary change
Hi @gabrielschulhof , Your changes have been introduced in 23b57cc. PTAL! |
Introduce
NODE_ADDON_API_REQUIRE_BASIC_FINALIZERSpreprocessor directive, which adds an always-fail static assertion inside theWrapperdefinition (responsible for passing the user's non-basic finalizer through anode_api_post_finalizercall) if this preprocessor directive is defined.