Uh oh!
There was an error while loading. Please reload this page.
src: begin restructuring node.cc for better maintainability - #20789
src: begin restructuring node.cc for better maintainability#20789jasnell wants to merge 3 commits into
Conversation
The prior commit causes a linting issue here. Fix by changing from a struct to a class.
jasnell
commented
May 16, 2018
mhdawson
commented
May 16, 2018
To help in reviewing, does this change code or simply move to a new file. If there is code that had to be changed in more than a trivial way and it is not the bulk of the new files it would be useful to identify those portions. |
jasnell
commented
May 16, 2018
The first and third commits simply move things around. There are no implementation changes. |
addaleax
left a comment
There was a problem hiding this comment.
LGTM, assuming you are volunteering to do backports :)
jasnell
commented
May 16, 2018
Yes, I'll do the backports for 8 and 6 |
TimothyGu
commented
May 16, 2018
Code changes LGTM. Do we need to make the new files inherit the license header in |
jasnell
commented
May 16, 2018
@TimothyGu ... no, the requirement for the license header was only maintaining it in the original files. New files do not need it. I know it's weird, but that's the agreement we came to. |
jasnell
commented
May 17, 2018
@addaleax ... looking at the backport to 8... it's going to be quite a bit more involved because of the |
addaleax
commented
May 17, 2018
@jasnell Makes sense … but we can backport the exceptions stuff, right? |
jasnell
commented
May 17, 2018
We ought to be able to do at least part of it, yeah, would just need to be selective about which pieces. |
PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
The prior commit causes a linting issue here. Fix by changing from a struct to a class. PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
jasnell
commented
May 19, 2018
Trott
commented
May 20, 2018
PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
The prior commit causes a linting issue here. Fix by changing from a struct to a class. PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
PR-URL: #20789 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
richardlau
commented
May 24, 2018
This had broken addons because the new headers referenced in |
Begins separating logical chunks of node.cc out into separate cc/h files for the purpose
of making these easier to maintain and making node.cc less of a monolithic jumble of code.
There is quite a bit in node.cc that can be divided into logical chunks and this PR only handles two of those. Before going too much further with it tho, I'd like to make sure folks are on board with this.
The eventual goal is to refactor the bootstrap model for node.js with an eye towards improving start up times, supporting snapshots, and generally making the bootstrap more maintainable. However, because there is so much bundled into node.cc, it is difficult to refactor and reviewing changes to make sure nothing breaks is quite challenging.
And yes, the idea would be to backport these changes to LTS lines as well.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes