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: reduce scope of variables throughout codebase #23297

Merged
merged 7 commits into from
Oct 9, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Oct 6, 2018

Move various variables in the C++ code to more narrow scopes where possible.

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

@nodejs-github-bot nodejs-github-bot added 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. labels Oct 6, 2018
Copy link
Member

@joyeecheung joyeecheung left a comment

Choose a reason for hiding this comment

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

Should we document about this preference in the style guide?

@refack
Copy link
Contributor

refack commented Oct 7, 2018

@joyeecheung
Copy link
Member

joyeecheung commented Oct 7, 2018

@refack Do we have any reference to the C++ Core Guidelines in our style guide? IIRC we only have an implicit reference to Googles style guide through cpplint(which is not able to check this)

@refack
Copy link
Contributor

refack commented Oct 7, 2018

Do we have any reference to the C++ Core Guidelines in our style guide? IIRC we only have an implicit reference to Googles style guide through cpplint(which is not able to check this)

Added a draft for discussion: #23317


do {
r = uv_fs_read(uv_default_loop(),
int r = uv_fs_read(uv_default_loop(),
&req,
file,
&buf,
Copy link
Member

Choose a reason for hiding this comment

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

Alignment. Some of these you can now make const.

reinterpret_cast<const unsigned char*>(sig),
siglen,
m,
m_len) == 1;
Copy link
Member

Choose a reason for hiding this comment

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

*verify_result = r == 1 is more readable, IMO.

if (fd_ != -1) {
err = uv_fs_close(nullptr, &req, fd_, nullptr);
CHECK_EQ(err, 0);
CHECK_EQ(uv_fs_close(nullptr, &req, fd_, nullptr), 0);
Copy link
Member

Choose a reason for hiding this comment

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

This is more commonly (27 vs. 11 instances) written as:

CHECK_EQ(0, uv_fs_close(nullptr, &req, fd_, nullptr));

@cjihrig cjihrig force-pushed the scope branch 2 times, most recently from 1229277 to 311cf25 Compare October 9, 2018 03:22
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs#23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Oct 9, 2018

Green (yellow) CI with nits addressed: https://ci.nodejs.org/job/node-test-pull-request/17697/

@cjihrig cjihrig merged commit f7bfbb8 into nodejs:master Oct 9, 2018
@cjihrig cjihrig deleted the scope branch October 9, 2018 12:36
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
targos pushed a commit that referenced this pull request Oct 10, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
PR-URL: #23297
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Joyee Cheung <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Gabriel Schulhof <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants