Skip to content

src: move more to node_process.cc from node.cc#22422

Closed
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:move-more-process
Closed

src: move more to node_process.cc from node.cc#22422
jasnell wants to merge 1 commit intonodejs:masterfrom
jasnell:move-more-process

Conversation

@jasnell
Copy link
Member

@jasnell jasnell commented Aug 20, 2018

Incrementally moving more stuff for the process object over to node_process.cc

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 20, 2018

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 20, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

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

@jasnell jasnell requested a review from addaleax August 21, 2018 13:36
refack
refack previously approved these changes Aug 21, 2018
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Rubber Stamp

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 21, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

Related failures in CI. Will revisit later today.

refack
refack reviewed Aug 21, 2018
View reviewed changes
src/node_internals.h Outdated
Copy link
Contributor

@refack refack Aug 21, 2018

Choose a reason for hiding this comment

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

Any reason not to have a node_process.h. IIUC these function are only used in node.cc.

Copy link
Member Author

@jasnell jasnell Aug 21, 2018

Choose a reason for hiding this comment

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

Eventually everything in node.cc that relates to the process object should be moved over to node_process.cc. Once that is done, we shouldn't need the declarations in node_internals.h any more and definitely won't need a node_process.h. These definitions are transitional only since SetupProcessObject is still in node.cc. I won't move that until after @addaleax's options parser stuff lands.

src/node_internals.h Outdated
Copy link
Contributor

@refack refack Aug 21, 2018

Choose a reason for hiding this comment

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

Shouldn't these be extern? with the definition only in node_process.cc?

@refack refack dismissed their stale review August 21, 2018 15:55

have comments

@jasnell jasnell force-pushed the move-more-process branch from 88211d3 to de58897 Compare August 21, 2018 16:36
@jasnell
Copy link
Member Author

jasnell commented Aug 21, 2018

Updated CI with (hopefully) fixes: https://ci.nodejs.org/job/node-test-pull-request/16641/

addaleax
addaleax approved these changes Aug 21, 2018
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM (once @refack’s comment from #22422 (comment) is addressed)

@addaleax addaleax added process Issues and PRs related to the process subsystem. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Aug 21, 2018
@jasnell jasnell force-pushed the move-more-process branch from de58897 to a8bbb33 Compare August 22, 2018 03:10
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

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

@jasnell jasnell added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Aug 22, 2018
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

CI is good.

@jasnell jasnell force-pushed the move-more-process branch from a8bbb33 to 2019d35 Compare August 22, 2018 21:58
@jasnell
Copy link
Member Author

jasnell commented Aug 22, 2018

CI after rebase: https://ci.nodejs.org/job/node-test-pull-request/16688/

jasnell added a commit that referenced this pull request Aug 23, 2018
@jasnell
src: move more to node_process.cc from node.cc
264c99f 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member Author

jasnell commented Aug 23, 2018

Landed in 264c99f

@jasnell jasnell closed this Aug 23, 2018
addaleax pushed a commit that referenced this pull request Aug 28, 2018
@jasnell @addaleax
src: move more to node_process.cc from node.cc
25fe930 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
targos pushed a commit that referenced this pull request Sep 3, 2018
@jasnell @targos
src: move more to node_process.cc from node.cc
2767eba 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@targos targos mentioned this pull request Sep 5, 2018
Merged
targos pushed a commit that referenced this pull request Sep 6, 2018
@jasnell @targos
src: move more to node_process.cc from node.cc
5f392a1 
PR-URL: #22422
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
This was referenced Sep 6, 2018
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@refack refack refack approved these changes

@devsnek devsnek devsnek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jasnell @nodejs-github-bot @refack @addaleax @cjihrig @devsnek