-
-
Notifications
You must be signed in to change notification settings - Fork 33.9k
node-api: add support for Float16Array #58879
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
node-api: add support for Float16Array #58879
Conversation
|
Review requested:
|
48a7303 to
3d5bf56
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #58879 +/- ##
==========================================
+ Coverage 88.05% 88.56% +0.50%
==========================================
Files 704 704
Lines 207857 207861 +4
Branches 39964 40049 +85
==========================================
+ Hits 183030 184092 +1062
+ Misses 16806 15802 -1004
+ Partials 8021 7967 -54
🚀 New features to boost your workflow:
|
vmoroz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that the new Node-API changes follow the Node-API change requirements outlined in this document: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md
Note that the requirement that says "Should be implemented in at least one other VM implementation of Node.js." - it is currently being discussed to be revised towards something like "being implementable by other JS runtimes and JS engines" - see #58852. I.e. the new API like in this PR must be good as long as it is not V8-specific.
ABI breaking changes made in subsequent commits. Removing the approval until all issues can be resolved
|
@IlyasShabi ... to address the ABI breaking issues, you might take some inspiration from this duplicate PR #58889 |
addaleax
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels quite silly to put this behind an experimental flag – this seems more of a "fix" to account for the fact that the language now exposes a new variant, rather than a new feature.
legendecas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed on node-api meeting, for this PR to be able to be backported to previous LTS lines, we will need a new napi_status as napi_not_supported, so that on Node.js v22 and v20, the api napi_create_typedarray return napi_not_supported if napi_float16_array is requested, even if the addon is compiled with NAPI_EXPERIMENTAL, or newer napi version.
And for the enum value ABI stability, we will have discussions in #59085 resolved and unblock this PR.
fcec785 to
63f957e
Compare
legendecas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you
|
@KevinEady @vmoroz would you mind taking a look at this updated PR again? It is necessary to address the change requests before this can be landed. Thank you! |
|
Landed in 900d329 |
This PR aims to add support for Float16Array in n-api, addressing this issue
cc: @nodejs/node-api