node-api: add napi_float16_array #58889
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
|
There is an open PR for this: #58879 |
|
The other guy should have back-linked to the issue... from a quick glance, that PR breaks ABI; this one doesn't. |
|
Ok, the other PR can be updated to fix any remaining issues. I'd encourage providing review comments and suggestions there. |
This comment was marked as resolved.
This comment was marked as resolved.
|
Why? This one is perfect. It's not first come, first serve, right? |
| @@ -2267,6 +2267,7 @@ typedef enum { | |||
| napi_float64_array, | |||
| napi_bigint64_array, | |||
| napi_biguint64_array, | |||
| napi_float16_array, | |||
There was a problem hiding this comment.
Without breaking the ABI, we still need to mark this as NAPI_EXPERIMENTAL as mentioned in #58879 (comment) until this is been backported to all LTS lines.
In addition, we need a feature macro, to allow addons to detect a new feature: https://github.com/nodejs/node/blob/main/doc/contributing/adding-new-napi-api.md#process-and-approval.
There was a problem hiding this comment.
Without breaking the ABI
The problem with this in #58879 was that the enum value was added in the middle. Adding an extra enum value at the end, as it is done here, is generally not ABI-breaking since it does not change the numeric values of the other enum alternatives.
There was a problem hiding this comment.
I'm not saying this breaks ABI. My point is we need guard this with NAPI_EXPERIMENTAL (and a NAPI_VERSION bump soon in the next versions) and a feature macro to allow addons detect that APIs like napi_create_typedarray are supported with napi_float16_array at both build time and run time.
|
Certainly not first come first serve but there's also no harm at all in helping a newer contributor work through any remaining issues in their PR. And as @legendecas points out there are outstanding issues in the PR also. |
Fixes: #58873