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

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

Closed
wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

  • 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 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
src/node_api.cc Outdated
@@ -827,6 +827,339 @@ napi_status ConcludeDeferred(napi_env env,
return GET_RETURN_STATUS(env);
}

class TsFn: public node::AsyncResource {
Copy link
Member

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

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

node::Environment::GetCurrent(env->isolate)->CloseHandle(
reinterpret_cast<uv_handle_t*>(&async),
[] (uv_handle_t* handle) -> void {
Copy link
Member

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 :)

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
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

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

@gabrielschulhof
Copy link
Contributor Author

* 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

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

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Aug 31, 2018

@gabrielschulhof
Copy link
Contributor Author

Landed in 403df7c.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Aug 31, 2018
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof deleted the cleanup-tsfn branch August 31, 2018 19:54
targos pushed a commit that referenced this pull request Sep 1, 2018
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos pushed a commit that referenced this pull request Sep 6, 2018
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Dec 28, 2018
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 18, 2019
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
rvagg pushed a commit that referenced this pull request Feb 28, 2019
* 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 <[email protected]>
Reviewed-By: Kyle Farnung <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

6 participants