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

Jump to bottom

src: change guards to NAPI_VERSION > 5 #697

Closed
wants to merge 1 commit into from
Closed

src: change guards to NAPI_VERSION > 5 #697

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

Since we have made the decision that we shall include BigInt into
N-API 6, we can change the guards for the BigInt and BigInt-based
typed array wrappers accordingly, and end our reliance on guarding by
NODE_MAJOR_VERSION.

Signed-off-by: @gabrielschulhof

Supersedes #694.

@gabrielschulhof gabrielschulhof force-pushed the remove-NODE_MAJOR_VERSION-guards branch 2 times, most recently from 3acf151 to 4f19ef7 Compare April 7, 2020 22:05
@gabrielschulhof gabrielschulhof force-pushed the remove-NODE_MAJOR_VERSION-guards branch from 4f19ef7 to 0d23cd9 Compare April 7, 2020 22:32
legendecas
legendecas approved these changes Apr 8, 2020
View reviewed changes
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

NickNaso
NickNaso requested changes Apr 8, 2020
View reviewed changes
test/binding.cc Outdated Show resolved Hide resolved
test/binding.cc Outdated Show resolved Hide resolved
@gabrielschulhof gabrielschulhof force-pushed the remove-NODE_MAJOR_VERSION-guards branch from 0d23cd9 to 5e0bcd7 Compare April 10, 2020 05:08
@gabrielschulhof
Copy link
Contributor Author

@NickNaso I removed the extra comments.

NickNaso
NickNaso previously approved these changes Apr 10, 2020
View reviewed changes
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

@gabrielschulhof
Copy link
Contributor Author

@NickNaso looks like your subsequent approval did not clear your previous request for changes. Can you please dismiss?

@NickNaso NickNaso dismissed their stale review April 10, 2020 16:12

My approval did not clear changes that I requested.

@gabrielschulhof
Copy link
Contributor Author

@NickNaso looks like dismissing your change request also dismissed your approval. Could you please re-approve?

NickNaso
NickNaso approved these changes Apr 11, 2020
View reviewed changes
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

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 13, 2020 •
edited
Loading

Tests:

Version Job Result
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2009/ ✗
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2010/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2011/ ✓

@mhdawson mhdawson mentioned this pull request Apr 13, 2020
Closed
8 tasks
@gabrielschulhof gabrielschulhof force-pushed the remove-NODE_MAJOR_VERSION-guards branch from 5e0bcd7 to b737089 Compare April 13, 2020 15:11
@gabrielschulhof
Copy link
Contributor Author

On Windows it looks like one cannot distinguish between thread IDs using http://docs.libuv.org/en/v1.x/threading.html#c.uv_thread_equal.

@gabrielschulhof
Copy link
Contributor Author

nodejs/node#32823 should fix this problem.

mhdawson
mhdawson approved these changes Apr 14, 2020
View reviewed changes
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

@gabrielschulhof gabrielschulhof mentioned this pull request Apr 15, 2020
Closed
2 tasks
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 16, 2020
Revert "n-api: detect deadlocks in thread-safe function"
2a7b09c 
This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)

Signed-off-by: Gabriel Schulhof <[email protected]>
@gabrielschulhof gabrielschulhof mentioned this pull request Apr 16, 2020
Closed
4 tasks
gabrielschulhof pushed a commit to nodejs/node that referenced this pull request Apr 16, 2020
Revert "n-api: detect deadlocks in thread-safe function"
d3d5eca 
This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #32880
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
MylesBorins pushed a commit to nodejs/node that referenced this pull request Apr 17, 2020
@MylesBorins
Revert "n-api: detect deadlocks in thread-safe function"
076ba31 
This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #32880
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@legendecas legendecas mentioned this pull request Apr 20, 2020
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Apr 20, 2020
Revert "n-api: detect deadlocks in thread-safe function"
62c953e 
This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs#32880
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@NickNaso
Copy link
Member

@gabrielschulhof Could I merge this PR?

@gabrielschulhof
Copy link
Contributor Author

@NickNaso I don't believe the tests will pass on 14, 13, 12, and 10 without nodejs/node#32948.

src: change guards to NAPI_VERSION > 5
9ace39e 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
@gabrielschulhof gabrielschulhof force-pushed the remove-NODE_MAJOR_VERSION-guards branch from b737089 to 9ace39e Compare April 24, 2020 22:27
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 24, 2020 •
edited
Loading

CI:

Version Job Status
v15.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2071/ ✓
v14.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2072/ ✓
v12.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2073/ ✓
v10.x https://ci.nodejs.org/job/node-test-node-addon-api-new/2074/ ✓

gabrielschulhof pushed a commit that referenced this pull request Apr 24, 2020
src: change guards to NAPI_VERSION > 5
82a9650 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #697
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in 82a9650.

targos pushed a commit to nodejs/node that referenced this pull request Apr 28, 2020
@targos
Revert "n-api: detect deadlocks in thread-safe function"
cc0395c 
This reverts commit aeb7084.

The solution creates incorrect behaviour on Windows.

Re: nodejs/node-addon-api#697 (comment)
Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: #32880
Backport-PR-URL: #32948
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Gerhard Stöbich <[email protected]>
@gabrielschulhof gabrielschulhof deleted the remove-NODE_MAJOR_VERSION-guards branch February 1, 2021 16:39
@gabrielschulhof gabrielschulhof restored the remove-NODE_MAJOR_VERSION-guards branch February 1, 2021 16:40
@gabrielschulhof gabrielschulhof deleted the remove-NODE_MAJOR_VERSION-guards branch March 7, 2021 18:44
kevindavies8 added a commit to kevindavies8/node-addon-api-Develop that referenced this pull request Aug 24, 2022
@kevindavies8
src: change guards to NAPI_VERSION > 5
cafba95 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Marlyfleitas added a commit to Marlyfleitas/node-api-addon-Development that referenced this pull request Aug 26, 2022
@Marlyfleitas
src: change guards to NAPI_VERSION > 5
d277071 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
wroy7860 added a commit to wroy7860/addon-api-benchmark-node that referenced this pull request Sep 19, 2022
@wroy7860
src: change guards to NAPI_VERSION > 5
0fa84ce 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
johnfrench3 pushed a commit to johnfrench3/node-addon-api-git that referenced this pull request Aug 11, 2023
src: change guards to NAPI_VERSION > 5
9d6622e 
Since we have made the decision that we shall include `BigInt` into
N-API 6, we can change the guards for the `BigInt` and `BigInt`-based
typed array wrappers accordingly, and end our reliance on guarding by
`NODE_MAJOR_VERSION`.

Signed-off-by: Gabriel Schulhof <[email protected]>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas legendecas approved these changes

@NickNaso NickNaso NickNaso approved these changes

@KevinEady KevinEady Awaiting requested review from KevinEady

@devsnek devsnek Awaiting requested review from devsnek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gabrielschulhof @NickNaso @legendecas @mhdawson