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.

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

support bigint64/biguint64 in TypedArray::ElementSize() #705

Closed

Conversation

fs-eire
Copy link
Contributor

@fs-eire fs-eire commented Apr 18, 2020

No description provided.

Copy link
Member

@legendecas legendecas left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

napi-inl.h Outdated
@@ -1628,6 +1628,10 @@ inline uint8_t TypedArray::ElementSize() const {
case napi_float32_array:
return 4;
case napi_float64_array:
#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

N-API BigInt support has been out of experimental and can be guarded with

#if (NAPI_VERSION >= 6)

Check out #697 for more contexts.

@mhdawson mhdawson mentioned this pull request Apr 20, 2020
8 tasks
napi-inl.h Outdated
@@ -1628,6 +1628,10 @@ inline uint8_t TypedArray::ElementSize() const {
case napi_float32_array:
return 4;
case napi_float64_array:
#ifdef NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#ifdef NAPI_EXPERIMENTAL
#if (NAPI_VERSION > 5)

napi-inl.h Outdated
#ifdef NAPI_EXPERIMENTAL
case napi_bigint64_array:
case napi_biguint64_array:
#endif // NAPI_EXPERIMENTAL
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
#endif // NAPI_EXPERIMENTAL
#endif // NAPI_VERSION > 5

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM after the changes proposed by @mhdawson will be addressed.

Copy link
Member

@NickNaso NickNaso left a comment

Choose a reason for hiding this comment

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

LGTM. @fs-eire Thanks for your contribution.

Copy link
Contributor

@gabrielschulhof gabrielschulhof left a comment

Choose a reason for hiding this comment

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

LGTM % nit

napi-inl.h Outdated Show resolved Hide resolved
Co-Authored-By: Gabriel Schulhof <[email protected]>
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

mhdawson pushed a commit that referenced this pull request Apr 24, 2020
PR-URL: #705
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
@mhdawson
Copy link
Member

Landed as 4de23c9

@mhdawson mhdawson closed this Apr 24, 2020
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
PR-URL: nodejs/node-addon-api#705
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
PR-URL: nodejs/node-addon-api#705
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
PR-URL: nodejs/node-addon-api#705
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
PR-URL: nodejs/node-addon-api#705
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Nicola Del Gobbo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants