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

[WIP] src: introduce node_bootstrap and node_process #20879

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented May 22, 2018

Furthering the cause of breaking node.cc up into smaller, more manageable chunks.

This does two main things:

  1. Adds a new bootstrapper object that is used only during bootstrap to avoid having to delete and overwrite properties on process during bootstrap

  2. Moves most of the process function and property definitions to node_process.cc for better isolation.

Functionality is the same, tho there are some code cleanups along the way. This should be semver-patch.

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

This continues the reorganization of node.cc by splitting the
implementations of most process object functions and properties
out to a separate node_process.cc and creates a separate
temporary bootstrapper object used during Node.js bootstrap
to avoid the need for deleting/overwriting process properties
set during bootstrap.
Various cleanups within SetupProcessObject to improve readability
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label May 22, 2018
@jasnell jasnell requested a review from addaleax May 22, 2018 02:39
@trivikr trivikr added the c++ Issues and PRs that require attention from people who are familiar with C++. label May 22, 2018
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

JS changes LGTM.
Rubber stamp LGTM on C++ changes, assuming it's mostly cut and paste.
Big +1 on reducing the number of lines in node.cc!

@targos
Copy link
Member

targos commented May 22, 2018

@addaleax
Copy link
Member

Can we do this in smaller chunks, more slowly? Landing a patch this size is going to be a major source of conflicts, and not just with things like #20876

@jasnell
Copy link
Member Author

jasnell commented May 22, 2018

Yeah, splitting it up is possible but will take awhile longer. I'd like to get agreement on this path before staring to do that. :)

@jasnell
Copy link
Member Author

jasnell commented May 23, 2018

@addaleax ... before I start splitting this up, can I ask you to give me a better sense for how it should be split up so the effort will have the least dramatic impact on what you're working on with the Workers stuff.

@jasnell
Copy link
Member Author

jasnell commented May 23, 2018

Starting to break things up a bit: #20917

@addaleax
Copy link
Member

@jasnell Yes, that smaller PR looks better with regard to conflicts :)

In general, the easiest thing to check for how much "conflict potential" there is would be applying on PRs on top of the other… taking it in small chunks is definitely the right approach here

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label May 24, 2018
@jasnell jasnell changed the title src: introduce node_bootstrap and node_process [WIP] src: introduce node_bootstrap and node_process May 24, 2018
@jasnell jasnell closed this May 30, 2018
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++. lib / src Issues and PRs related to general changes in the lib or src directory. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants