Skip to content

src: change guards to NAPI_VERSION > 5#697

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:remove-NODE_MAJOR_VERSION-guards
Closed

src: change guards to NAPI_VERSION > 5#697
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:remove-NODE_MAJOR_VERSION-guards

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Apr 6, 2020

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
@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

gabrielschulhof commented Apr 10, 2020

@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

gabrielschulhof commented Apr 10, 2020

@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

gabrielschulhof commented Apr 11, 2020

@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

gabrielschulhof commented Apr 13, 2020

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

gabrielschulhof commented Apr 13, 2020

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 <gabriel.schulhof@intel.com>
@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 <gabriel.schulhof@intel.com>
PR-URL: #32880
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
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 <gabriel.schulhof@intel.com>
PR-URL: #32880
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@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 <gabriel.schulhof@intel.com>
PR-URL: nodejs#32880
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@NickNaso
Copy link
Member

NickNaso commented Apr 23, 2020

@gabrielschulhof Could I merge this PR?

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 23, 2020

@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 <gabriel.schulhof@intel.com>
@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 <gabriel.schulhof@intel.com>
PR-URL: #697
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Apr 24, 2020

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 <gabriel.schulhof@intel.com>
PR-URL: #32880
Backport-PR-URL: #32948
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
@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 <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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 <gabriel.schulhof@intel.com>
PR-URL: nodejs/node-addon-api#697
Reviewed-By: Nicola Del Gobbo <nicoladelgobbo@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NickNaso NickNaso NickNaso approved these changes

@legendecas legendecas legendecas approved these changes

@mhdawson mhdawson mhdawson 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