Skip to content

n-api: clean up thread-safe function#22259

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:cleanup-tsfn
Closed

n-api: clean up thread-safe function#22259
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:cleanup-tsfn

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Aug 11, 2018

  • Move class TsFn to name space v8impl
  • Remove NAPI_EXTERN from API declarations, because it's only needed
    in the header file.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 11, 2018

@gabrielschulhof build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/515/pipeline

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API. labels Aug 11, 2018
addaleax
addaleax approved these changes Aug 22, 2018
View reviewed changes
src/node_api.cc Outdated
Copy link
Member

@addaleax addaleax Aug 22, 2018

Choose a reason for hiding this comment

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

style nits: I'd very much prefer to spell out ThreadSafeFunction, and we use spaces before the : in a class declaration

If you need abbreviations for individual functions, I think an using line might do the trick

Copy link
Member

@jasnell jasnell Aug 23, 2018

Choose a reason for hiding this comment

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

Yes, please avoid abbreviations

src/node_api.cc Outdated
Copy link
Member

@addaleax addaleax Aug 22, 2018

Choose a reason for hiding this comment

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

style nit: No space after []

Aside, we have make format-cpp (possibly preceded by make format-cpp-build), that should take care of style issues now :)

mhdawson
mhdawson approved these changes Aug 23, 2018
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 once @addaleax comments are addressed.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 23, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16697/

@addaleax @mhdawson nits addressed.

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 23, 2018

Resumed as: https://ci.nodejs.org/job/node-test-pull-request/16702/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 24, 2018

Another CI from the top: https://ci.nodejs.org/job/node-test-pull-request/16731/

@addaleax addaleax mentioned this pull request Aug 25, 2018
Closed
4 tasks
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 25, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/16753/

n-api: clean up thread-safe function
2790896 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 28, 2018

Wow! git show --patience makes a huge difference!

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 28, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16829/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 28, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/16834/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 28, 2018

Resumed as https://ci.nodejs.org/job/node-test-pull-request/16837/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 31, 2018

Another CI: https://ci.nodejs.org/job/node-test-pull-request/16902/

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 31, 2018 •
edited
Loading

Resumed as CI: https://ci.nodejs.org/job/node-test-pull-request/16915/ ✓

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 31, 2018

Landed in 403df7c.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Aug 31, 2018
n-api: clean up thread-safe function
403df7c 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: nodejs#22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@gabrielschulhof gabrielschulhof deleted the cleanup-tsfn branch August 31, 2018 19:54
targos pushed a commit that referenced this pull request Sep 1, 2018
@targos
n-api: clean up thread-safe function
aecf4e0 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: #22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
targos pushed a commit that referenced this pull request Sep 3, 2018
@targos
n-api: clean up thread-safe function
96213c8 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: #22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@targos
n-api: clean up thread-safe function
cf1a020 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: #22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
This was referenced Sep 6, 2018
Closed
Closed
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
n-api: clean up thread-safe function
35c6b1d 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

PR-URL: nodejs#22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
@MylesBorins
n-api: clean up thread-safe function
a0fa101 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

Backport-PR-URL: #25002
PR-URL: #22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
@rvagg
n-api: clean up thread-safe function
902b079 
* Move class `TsFn` to name space `v8impl` and rename it to
  `ThreadSafeFunction`
* Remove `NAPI_EXTERN` from API declarations, because it's only needed
  in the header file.

Backport-PR-URL: #25002
PR-URL: #22259
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@MylesBorins MylesBorins mentioned this pull request Mar 26, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments

@addaleax addaleax addaleax approved these changes

@mhdawson mhdawson mhdawson approved these changes

+1 more reviewer

@kfarnung kfarnung kfarnung approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gabrielschulhof @nodejs-github-bot @jasnell @addaleax @kfarnung @mhdawson