X Tutup
The Wayback Machine - https://web.archive.org/web/20220409063005/https://github.com/nodejs/node/pull/42563
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

os: avoid unnecessary usage of var #42563

Merged
merged 1 commit into from Apr 3, 2022

Conversation

Copy link
Member

@VoltrexMaster VoltrexMaster commented Apr 1, 2022

The var keyword is known to be problematic and is not needed here,
so better to use the let keyword for variable declarations.

The `var` keyword is known to be problematic and is not needed here,
so better to use the `let` keyword for variable declarations.
@nodejs-github-bot nodejs-github-bot added needs-ci os labels Apr 1, 2022
Copy link
Member

@tniessen tniessen left a comment

I'm surprised to see we still have some var declarations (assuming there are no performance edge cases here, but I think V8 fixed those years ago).

@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Apr 1, 2022

Running the os benchmarks to be sure because we have a benchmark test for

networkInterfaces();

which we are touching in this PR and we still have comments like
// Using var here instead of let because "for (var ...)" is faster than let.
// Refs: https://github.com/nodejs/node/pull/30380#issuecomment-552948364

https://ci.nodejs.org/job/benchmark-node-micro-benchmarks/1115/

                                confidence improvement accuracy (*)   (**)  (***)
os/cpus.js n=30000                              0.00 %       ±0.68% ±0.91% ±1.18%
os/loadavg.js n=5000000                        -0.40 %       ±1.17% ±1.55% ±2.02%
os/networkInterfaces.js n=10000                -2.51 %       ±4.75% ±6.32% ±8.23%

I guess, we can replace all the vars with let/consts.

Copy link
Member

@RaisinTen RaisinTen left a comment

LGTM

@marsonya marsonya added author ready and removed author ready labels Apr 1, 2022
Trott
Trott approved these changes Apr 2, 2022
@VoltrexMaster VoltrexMaster added the request-ci label Apr 2, 2022
@github-actions github-actions bot removed the request-ci label Apr 2, 2022
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 2, 2022

@RaisinTen RaisinTen added the author ready label Apr 2, 2022
@VoltrexMaster VoltrexMaster removed the needs-ci label Apr 2, 2022
@Trott Trott added the needs-ci label Apr 3, 2022
@Trott
Copy link
Member

@Trott Trott commented Apr 3, 2022

A bit of an aside, but I've re-added the needs-ci label.

It's perhaps a bit confusingly named, but the needs-ci label is there for the commit queue automation. It tells the automation that this is a PR that should land only if there is a passing Jenkins CI run. It's not a label letting other people know that we need a CI run. So it should generally not be manually removed (unless it was incorrectly placed on a PR in the first place.)

Refs: https://github.com/nodejs/node-core-utils/blob/919ec3b784a2589aa88a1b2629b98d57283555a4/lib/pr_checker.js#L431-L438

@Trott Trott added the commit-queue label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue label Apr 3, 2022
@nodejs-github-bot nodejs-github-bot merged commit b5f0b49 into nodejs:master Apr 3, 2022
63 checks passed
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 3, 2022

Landed in b5f0b49

@VoltrexMaster VoltrexMaster deleted the avoid-var branch Apr 3, 2022
juanarbol added a commit to juanarbol/node that referenced this issue Apr 5, 2022
The `var` keyword is known to be problematic and is not needed here,
so better to use the `let` keyword for variable declarations.

PR-URL: nodejs#42563
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request Apr 5, 2022
@juanarbol juanarbol mentioned this pull request Apr 5, 2022
juanarbol added a commit that referenced this issue Apr 6, 2022
The `var` keyword is known to be problematic and is not needed here,
so better to use the `let` keyword for variable declarations.

PR-URL: #42563
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Akhil Marsonya <akhil.marsonya27@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready needs-ci os
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants
X Tutup