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

src: updating references to the old node.js file #8092

Closed
wants to merge 2 commits into from

Conversation

danbev
Copy link
Contributor

@danbev danbev commented Aug 13, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

Description of change

The node.js script was renamed to bootstrap_node.js as part of
81b6882. When I did this I missed
several comments in src/node.cc that referred to the old file name.

This commit updates the comments to refer to bootstrap_node.js and
correct the path to this file where used.
It also moves a comment that seems to have drifted in the file.

The node.js script was renamed to bootstrap_node.js as part of
81b6882. When I did this I missed
several comments in src/node.cc that referred to the old file name.

This commit updates the comments to refer to bootstrap_node.js and
correct the path to this file where used.
It also moves a comment that seems to have drifted in the file.
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Aug 13, 2016

// We start the process this way in order to be more modular. Developers
// who do not like how 'src/node.js' setups the module system but do like
// Node's I/O bindings may want to replace 'f' with their own function.
// who do not like how 'lib/internal/bootstrap_node.js' setups the module
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps putting the full path here is unnecessary since the filename itself isn't ambiguous?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, I'll remove the path.

CHECK(f_value->IsFunction());
Local<Function> f = Local<Function>::Cast(f_value);

// Now we call 'f' with the 'process' variable that we've built up with
// all our bindings. Inside node.js we'll take care of assigning things to
// their places.
// all our bindings. Inside bootstrap_node.js we'll take care of
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be "internal/bootstrap_node.js".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I might have misunderstood @mscdex comment about the full path as I removed it. I'm happy to change this, but at this point I'm not exactly sure if I should add internal to all references or not.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's necessary to put the full path

Copy link
Contributor

Choose a reason for hiding this comment

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

same

@Fishrock123
Copy link
Contributor

lgtm

@mscdex
Copy link
Contributor

mscdex commented Aug 14, 2016

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Aug 15, 2016

LGTM

jasnell pushed a commit that referenced this pull request Aug 18, 2016
The node.js script was renamed to bootstrap_node.js as part of
81b6882. Several comments were
missed in src/node.cc that referred to the old file name.

This commit updates the comments to refer to bootstrap_node.js and
correct the path to this file where used.
It also moves a comment that seems to have drifted in the file.

PR-URL: #8092
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Aug 18, 2016

Landed in 0441f2a

@jasnell jasnell closed this Aug 18, 2016
evanlucas pushed a commit that referenced this pull request Aug 24, 2016
The node.js script was renamed to bootstrap_node.js as part of
81b6882. Several comments were
missed in src/node.cc that referred to the old file name.

This commit updates the comments to refer to bootstrap_node.js and
correct the path to this file where used.
It also moves a comment that seems to have drifted in the file.

PR-URL: #8092
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@danbev danbev deleted the updating-node.js-comments branch February 23, 2017 07:55
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++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants