Skip to content

src: move InternalMakeCallback and MakeCallback into callback_scope.cc - #25299

Closed
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:make-callback
Closed

src: move InternalMakeCallback and MakeCallback into callback_scope.cc#25299
joyeecheung wants to merge 1 commit into
nodejs:masterfrom
joyeecheung:make-callback

Conversation

@joyeecheung

@joyeecheungjoyeecheung commented Jan 1, 2019

Copy link
Copy Markdown
Member

Since these are just wrappers on top of InternalCallbackScope, this
makes the implementations easier to find.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot

nodejs-github-bot commented Jan 1, 2019

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-botnodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 1, 2019
@joyeecheung

joyeecheung commented Jan 1, 2019•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

hmm, or maybe an alternative solution is to put these in an api.cc ? @addaleax But that may result in another monolithic file as well..

EDIT: or maybe a src/api/ folder would work better?

addaleax
addaleax approved these changes Jan 1, 2019
View reviewed changes

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it’s fine this way 👍

@joyeecheung

joyeecheung commented Jan 4, 2019

Copy link
Copy Markdown
MemberAuthor

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

@joyeecheungjoyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 4, 2019
@addaleax

addaleax commented Jan 7, 2019

Copy link
Copy Markdown
Member

This needs a rebase

@addaleaxaddaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 7, 2019
@joyeecheung
src: move InternalMakeCallback and MakeCallback into callback_scope.cc
e301ca8
Since these are just wrappers on top of `InternalCallbackScope`, this
makes the implementations easier to find.
@joyeecheung

joyeecheung commented Jan 8, 2019

Copy link
Copy Markdown
MemberAuthor

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

@danbev

danbev commented Jan 9, 2019

Copy link
Copy Markdown
Contributor

Landed in e54d11e.

@danbevdanbev closed this Jan 9, 2019
danbev pushed a commit that referenced this pull request Jan 9, 2019
@joyeecheung@danbev
src: move InternalMakeCallback and MakeCallback
e54d11e
This commit moves InternalMakeCallback and MakeCallback into
callback_scope.cc. Since these are just wrappers on top of
`InternalCallbackScope`, this makes the implementations easier to find.
#25299
PR-URL: #25299
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 15, 2019
@joyeecheung@addaleax
src: move InternalMakeCallback and MakeCallback
0095f49
This commit moves InternalMakeCallback and MakeCallback into
callback_scope.cc. Since these are just wrappers on top of
`InternalCallbackScope`, this makes the implementations easier to find.
#25299
PR-URL: #25299
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeARBridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@joyeecheung@BridgeAR
src: move InternalMakeCallback and MakeCallback
b016fd9
This commit moves InternalMakeCallback and MakeCallback into
callback_scope.cc. Since these are just wrappers on top of
`InternalCallbackScope`, this makes the implementations easier to find.
nodejs#25299
PR-URL: nodejs#25299
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 17, 2019
@joyeecheung@BridgeAR
src: move InternalMakeCallback and MakeCallback
dca6741
This commit moves InternalMakeCallback and MakeCallback into
callback_scope.cc. Since these are just wrappers on top of
`InternalCallbackScope`, this makes the implementations easier to find.
nodejs#25299
PR-URL: nodejs#25299
Reviewed-By: Gus Caplan <me@gus.host>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2019
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@addaleaxaddaleaxaddaleax approved these changes
+1 more reviewer
@devsnekdevsnekdevsnek 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++.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@joyeecheung@nodejs-github-bot@addaleax@danbev@jasnell@devsnek