Skip to content

n-api: clean up binding creation#36170

Closed
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:simplify-callback
Closed

n-api: clean up binding creation#36170
gabrielschulhof wants to merge 1 commit intonodejs:masterfrom
gabrielschulhof:simplify-callback

Conversation

@gabrielschulhof
Copy link
Contributor

@gabrielschulhof gabrielschulhof commented Nov 19, 2020

  • Remove dead code for GetterCallbackWrapper and
    SetterCallbackWrapper.
  • Factor out creation of new v8::Functions.
  • Factor out creation of new v8::FunctionTemplates.
  • Turn CallbackBundle into a class, internalizing creation of new
    instances and garbage collection.

Signed-off-by: @gabrielschulhof

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 Nov 19, 2020 •
edited by mhdawson
Loading

Review requested:

  • @nodejs/n-api

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Nov 19, 2020
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Nov 19, 2020
addaleax
addaleax approved these changes Nov 19, 2020
View reviewed changes
n-api: clean up binding creation
26ee991 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: nodejs#36170
mhdawson
mhdawson approved these changes Nov 20, 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 added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 20, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 20, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Nov 20, 2020

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

gabrielschulhof pushed a commit that referenced this pull request Nov 21, 2020
n-api: clean up binding creation
03fd363 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@gabrielschulhof
Copy link
Contributor Author

gabrielschulhof commented Nov 21, 2020

Landed in 03fd363.

codebytere pushed a commit that referenced this pull request Nov 22, 2020
@codebytere
n-api: clean up binding creation
79b2ba6 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@codebytere codebytere mentioned this pull request Nov 22, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@BethGriggs
n-api: clean up binding creation
ac08dc8 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
@BethGriggs
n-api: clean up binding creation
b42a84f 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@BethGriggs BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@BethGriggs
n-api: clean up binding creation
81b0562 
* Remove dead code for `GetterCallbackWrapper` and
  `SetterCallbackWrapper`.
* Factor out creation of new `v8::Function`s.
* Factor out creation of new `v8::FunctionTemplate`s.
* Turn `CallbackBundle` into a class, internalizing creation of new
  instances and garbage collection.

Signed-off-by: Gabriel Schulhof <gabriel.schulhof@intel.com>
PR-URL: #36170
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: David Carlier <devnexen@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@gabrielschulhof gabrielschulhof deleted the simplify-callback branch January 28, 2021 00:14
@gabrielschulhof gabrielschulhof restored the simplify-callback branch January 28, 2021 05:40
@gabrielschulhof gabrielschulhof deleted the simplify-callback branch February 3, 2021 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@Trott Trott Trott approved these changes

@addaleax addaleax addaleax approved these changes

@mhdawson mhdawson mhdawson approved these changes

@legendecas legendecas Awaiting requested review from legendecas

+1 more reviewer

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

7 participants

@gabrielschulhof @nodejs-github-bot @jasnell @Trott @addaleax @devnexen @mhdawson